Single Findings API Endpoint #1516
Conversation
WalkthroughThis update introduces a new API endpoint for retrieving detailed attack path findings, along with supporting schemas and documentation updates. The SQL filter construction logic in the backend was refactored to use a typed AST-based approach; related unit tests were consolidated and updated to match the new SQL filter string format. A new audit log action constant was added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant DB
Client->>API_Server: PUT /api/v2/attack-paths/details
API_Server->>DB: Query all attack path findings and assets
DB-->>API_Server: Findings and assets data
API_Server-->>Client: 200 OK (api.response.all-findings)
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (10)
🧰 Additional context used🧠 Learnings (4)📓 Common learningspackages/go/openapi/src/schemas/model.unified-finding.yaml (2)cmd/api/src/model/filter_test.go (4)packages/go/openapi/src/schemas/api.response.all-findings.yaml (1)🧬 Code Graph Analysis (1)cmd/api/src/model/filter_test.go (1)
🪛 YAMLlint (1.37.1)packages/go/openapi/src/schemas/model.unified-finding.yaml[error] 1-1: wrong new line character: expected \n (new-lines) packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml[error] 1-1: wrong new line character: expected \n (new-lines) packages/go/openapi/src/schemas/api.response.all-findings.yaml[error] 1-1: wrong new line character: expected \n (new-lines) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/model/audit.go (1)
76-79: Add documentation and tests for new audit log actionThe new
AuditLogActionExportAllRisksconstant follows the existing naming pattern. Please ensure you also:
- Update the OpenAPI enum/schema for audit logs to include
"ExportAllRisks".- Add or extend unit/integration tests to cover the new action in your logging workflows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/model/audit.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/go/openapi/src/schemas/api.response.all-findings.yaml (1)
1-16: License header is correct.
The Apache 2.0 license header is present and properly formatted.To satisfy the YAML linter error (
wrong new line character: expected \n), normalize line endings to LF (\n) across this file.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
1-16: License header is correct.
The Apache 2.0 header is correctly applied.Align line endings to LF (
\n) to resolve the YAML linting warning on new line characters.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/api/src/model/audit.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml(1 hunks)packages/go/openapi/src/schemas/api.response.all-findings.yaml(1 hunks)packages/javascript/bh-shared-ui/src/test-utils.jsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryCard.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryCard.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryCard.test.tsx
- packages/javascript/bh-shared-ui/src/utils/index.ts
- packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryCard.tsx
- packages/javascript/bh-shared-ui/src/test-utils.jsx
- packages/go/openapi/src/openapi.yaml
- packages/go/openapi/doc/openapi.json
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/model/audit.go
🧰 Additional context used
🪛 Biome (1.9.4)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
[error] 15-19: Expected a statement but instead found '<<<<<<< Updated upstream
Stashed changes'.
Expected a statement here.
(parse)
🪛 GitHub Actions: Build UI
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
[error] 16-16: SyntaxError: Merge conflict marker encountered. Conflict markers <<<<<<<, =======, >>>>>>> found in the file.
🪛 GitHub Actions: Static Code Analysis
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
[error] 16-16: Parsing error: Merge conflict marker encountered.
🪛 GitHub Actions: Run Tests
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
[error] 16-16: Transform failed: Unexpected '<<' found in source code indicating unresolved merge conflict markers.
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/schemas/api.response.all-findings.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
🔇 Additional comments (1)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
17-25: Verify use of PUT without a request body.
This endpoint usesPUTbut defines norequestBody. If no payload is required, usingGETis more semantically appropriate for a read operation. If you intend to accept a JSON body (e.g., to filter findings), add arequestBodydefinition.Please confirm that
PUTwithout a body aligns with the API design guidelines.
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
Outdated
Show resolved
Hide resolved
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts (1)
16-19:⚠️ Potential issueCritical: Unresolved Git merge conflict markers are still blocking the pipeline.
The merge conflict markers identified in the previous review remain unresolved and are causing multiple pipeline failures. This is preventing the PR from being merged and must be addressed immediately.
Please remove all conflict marker lines to resolve this issue.
🧰 Tools
🪛 Biome (1.9.4)
[error] 15-19: Expected a statement but instead found '<<<<<<< Updated upstream
Stashed changes'.
Expected a statement here.
(parse)
🪛 GitHub Actions: Build UI
[error] 16-16: SyntaxError: Merge conflict marker encountered. Conflict markers <<<<<<<, =======, >>>>>>> found in the file.
🪛 GitHub Actions: Run Tests
[error] 16-16: Transform failed: Unexpected '<<' found in source code, indicating unresolved merge conflict markers. Please resolve the conflict in this file.
🪛 GitHub Actions: Static Code Analysis
[error] 16-16: Parsing error: Merge conflict marker encountered.
packages/go/openapi/src/schemas/api.response.all-findings.yaml (1)
17-24: Define detailed schema for array and object properties and mark them as required.
Thefindingsarray lacks anitemsdefinition and thefinding_assetsobject is missing anadditionalPropertiesschema. Additionally, neither property is marked as required, which may break code generation or validation.Apply this patch:
type: object properties: findings: type: array + items: + $ref: '#/components/schemas/UnifiedFinding' description: List of unified findings. finding_assets: type: object + additionalProperties: + type: string + format: byte description: Directory structure of Base64 encoded assets related to each finding organized by finding name. +required: + - findings + - finding_assetspackages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
26-28:⚠️ Potential issueFix response object structure for HTTP 200.
Referencing a schema directly under the200response is invalid in OpenAPI 3.0. A proper response object must include adescriptionand acontentblock with the schema.Apply this patch:
responses: - 200: - $ref: './../schemas/api.response.all-findings.yaml' + 200: + description: Successful retrieval of all attack path findings. + content: + application/json: + schema: + $ref: './../schemas/api.response.all-findings.yaml'
🧹 Nitpick comments (2)
packages/go/openapi/src/schemas/api.response.all-findings.yaml (1)
1-1: Normalize newline characters to Unix-style LF.
YAMLlint reportswrong new line character: expected \n. Ensure this file uses LF line endings to avoid linter errors.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
1-1: Normalize newline characters to Unix-style LF.
YAMLlint reportswrong new line character: expected \n. Convert this file to use LF line endings.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/api/src/model/audit.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml(1 hunks)packages/go/openapi/src/schemas/api.response.all-findings.yaml(1 hunks)packages/javascript/bh-shared-ui/src/test-utils.jsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryCard.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryCard.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryCard.tsx
- packages/go/openapi/src/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/javascript/bh-shared-ui/src/utils/index.ts
- packages/javascript/bh-shared-ui/src/views/TierManagement/Summary/SummaryCard.test.tsx
- packages/javascript/bh-shared-ui/src/test-utils.jsx
- cmd/api/src/model/audit.go
- packages/go/openapi/doc/openapi.json
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/schemas/api.response.all-findings.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
🪛 Biome (1.9.4)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
[error] 15-19: Expected a statement but instead found '<<<<<<< Updated upstream
Stashed changes'.
Expected a statement here.
(parse)
🪛 GitHub Actions: Build UI
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
[error] 16-16: SyntaxError: Merge conflict marker encountered. Conflict markers <<<<<<<, =======, >>>>>>> found in the file.
🪛 GitHub Actions: Run Tests
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
[error] 16-16: Transform failed: Unexpected '<<' found in source code, indicating unresolved merge conflict markers. Please resolve the conflict in this file.
🪛 GitHub Actions: Static Code Analysis
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/types.ts
[error] 16-16: Parsing error: Merge conflict marker encountered.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/go/openapi/src/schemas/api.response.all-findings.yaml (3)
17-25: 🛠️ Refactor suggestionMark required properties at the root
Ensure bothfindingsandfinding_assetsare mandatory in the response:type: object properties: findings: … finding_assets: … +required: + - findings + - finding_assets
19-21: 🛠️ Refactor suggestionDefine
itemsschema forfindingsarray
Without anitemsdefinition, tools can’t infer the element type for the array, breaking codegen and validation.
Apply this diff:findings: type: array + items: + $ref: '#/components/schemas/UnifiedFinding' description: List of unified findings.
22-24: 🛠️ Refactor suggestionAdd
additionalPropertiesschema forfinding_assets
Currently the object has no constraints on its values. Specify that each asset entry is a Base64-encoded string:finding_assets: type: object + additionalProperties: + type: string + format: byte description: Directory structure of Base64 encoded assets related to each finding organized by finding name.packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
27-28: 🛠️ Refactor suggestionUse a full response object instead of direct schema
$ref
In OpenAPI 3, each response must includedescriptionandcontent.
Replace:responses: $ref: './../schemas/api.response.all-findings.yaml'With:
responses: description: Successful retrieval of all attack path findings. content: application/json: schema: $ref: './../schemas/api.response.all-findings.yaml'
🧹 Nitpick comments (2)
packages/go/openapi/src/schemas/api.response.all-findings.yaml (1)
1-1: Use Unix-style LF line endings
YAMLlint reports wrong new line characters. Convert CRLF to LF to satisfy the linter.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
1-1: Use Unix-style LF line endings
Ensure the file uses LF line endings to satisfy YAMLlint.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/api/src/model/audit.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml(1 hunks)packages/go/openapi/src/schemas/api.response.all-findings.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/go/openapi/src/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/model/audit.go
- packages/go/openapi/doc/openapi.json
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/schemas/api.response.all-findings.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
zinic
force-pushed
the
BP-1611
branch
3 times, most recently
from
e663ca0 to
279cb1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/api/src/model/filter.go (1)
198-268: Consider enhancing error messages for better debugging.The error messages could include the operator value for easier debugging.
Enhance the error message:
default: - return SQLFilter{}, fmt.Errorf("invalid operator specified") + return SQLFilter{}, fmt.Errorf("invalid operator specified: %s", filter.Operator)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/api/v2/model.go(1 hunks)cmd/api/src/model/audit.go(1 hunks)cmd/api/src/model/filter.go(2 hunks)cmd/api/src/model/filter_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/api/src/api/v2/model.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/model/audit.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/api/src/model/filter_test.go (1)
cmd/api/src/model/filter.go (4)
Filters(445-445)SQLFilter(96-99)Filter(440-443)BuildSQLFilter(198-268)
cmd/api/src/model/filter.go (3)
packages/go/cypher/models/pgsql/model.go (7)
Literal(175-179)NullLiteral(182-188)NewLiteral(190-195)AsLiteral(213-231)OptionalAnd(1210-1218)NewBinaryExpression(321-327)Identifier(587-587)packages/go/cypher/models/pgsql/pgtypes.go (3)
Int8(83-83)Float8(85-85)Boolean(86-86)packages/go/cypher/models/pgsql/operators.go (1)
OperatorLike(92-92)
🪛 golangci-lint (1.64.8)
cmd/api/src/model/filter.go
[error] 166-166: SA5000: assignment to nil map
(staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
cmd/api/src/model/filter.go (2)
172-194: Well-structured type parsing logic.The function correctly handles type conversion with appropriate fallback to string literals.
270-272: Clean delegation pattern for backward compatibility.The method appropriately delegates to the new implementation while maintaining the existing API.
zinic
force-pushed
the
BP-1611
branch
3 times, most recently
from
1989879 to
1935cd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
27-28: Response object must includedescriptionandcontent, not a bare schema$ref.OpenAPI 3 requires:
responses: description: Successful retrieval of all attack path findings. content: application/json: schema: $ref: './schemas/api.response.all-findings.yaml'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/api/v2/model.go(1 hunks)cmd/api/src/model/filter.go(2 hunks)cmd/api/src/model/filter_test.go(1 hunks)packages/go/cypher/models/pgsql/model.go(1 hunks)packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml(1 hunks)packages/go/openapi/src/schemas/api.response.all-findings.yaml(1 hunks)packages/go/openapi/src/schemas/model.unified-finding.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- cmd/api/src/api/v2/model.go
- packages/go/cypher/models/pgsql/model.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/model/filter_test.go
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/schemas/model.unified-finding.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/schemas/api.response.all-findings.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (5)
cmd/api/src/model/filter.go (5)
28-30: Well-structured imports for PostgreSQL AST support.The addition of PostgreSQL AST and formatting packages supports the transition to type-safe SQL generation, which is a significant security and maintainability improvement.
150-170: Clean conversion between filter models.The
ToFiltersModelmethod properly initializes the map and converts between the old and new filter models. The implementation correctly handles the conversion without data loss.
172-194: Robust type inference for PostgreSQL literals.The
filterValueAsPGLiteralfunction implements a sensible type inference hierarchy (null → int64 → float64 → bool → string) for converting string filter values to typed PostgreSQL literals. The fallback topgsql.AsLiteralensures compatibility with the PostgreSQL package's type system.
196-269: Excellent refactoring to AST-based SQL generation.This refactor represents a significant security and maintainability improvement:
- Security: Using AST generation prevents SQL injection vulnerabilities
- Correctness: The
format.SyntaxNodeensures syntactically correct SQL- Null handling: Proper use of
IS/IS NOToperators for null comparisons- Type safety: Leverages the PostgreSQL AST type system
The logic correctly maps filter operators to PostgreSQL operators and handles the special case of
ApproximatelyEqualswithLIKEand wildcard wrapping. The use ofpgsql.OptionalAndto combine expressions is clean and readable.
271-273: Clean delegation preserves backward compatibility.The updated method properly delegates to the new
BuildSQLFilterfunction while maintaining the existing interface, ensuring backward compatibility for existing callers.
zinic
force-pushed
the
BP-1611
branch
4 times, most recently
from
8ccddc8 to
341a431
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
packages/go/openapi/src/schemas/api.response.all-findings.yaml (2)
28-29:⚠️ Potential issueConstrain
finding_assetsvalues to Base64 strings.Using
type: objectwith no format prevents validation and codegen. Replace it with Base64-encoded strings:-additionalProperties: - type: object +additionalProperties: + type: string + format: byte
22-23:⚠️ Potential issueIncorrect
$refpath breaks schema resolution.The
itemsreference should point to the siblingmodel.unified-finding.yamldirectly.
Apply this diff to fix:-items: - $ref: './../schemas/model.unified-finding.yaml' +items: + $ref: './model.unified-finding.yaml'packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
27-28:⚠️ Potential issueFix response object to use
descriptionandcontent.OpenAPI requires a full response object, not a direct schema reference. Update the 200 response:
- 200: - $ref: './../schemas/api.response.all-findings.yaml' + 200: + description: Successful retrieval of all attack path findings. + content: + application/json: + schema: + $ref: './../schemas/api.response.all-findings.yaml'packages/go/openapi/src/schemas/model.unified-finding.yaml (3)
61-62:⚠️ Potential issueCorrect type for
attack_path_edge_id.OpenAPI requires
integerwithformat: int64for 64-bit IDs:-attack_path_edge_id: - type: int64 +attack_path_edge_id: + type: integer + format: int64
49-52:⚠️ Potential issueFix
target_propertiestype in the second variant.
additionalPropertiesonly applies to objects. Change the type accordingly:-target_properties: - type: string - additionalProperties: - type: object +target_properties: + type: object + additionalProperties: + type: object
75-77:⚠️ Potential issueUse numeric type for
exposure_percentage.Exposure percentages should be numbers, not strings:
-exposure_percentage: - type: string - format: int64 +exposure_percentage: + type: number + format: double
🧹 Nitpick comments (4)
cmd/api/src/database/sso_providers_test.go (1)
241-243: Consider maintaining parameterized query pattern for consistency.The change from parameterized queries (
"name = ?"with params) to direct string embedding reduces type safety and makes tests more brittle. While acceptable in test code, consider whether this aligns with the broader SQL filter handling strategy across the codebase.cmd/api/src/database/assetgrouptags_test.go (1)
444-444: Inconsistent approach compared to other changes in this file.This hardcoded
"type = 2"approach is inconsistent with lines 255 and 273 which usestrconv.Itoa(int(model.AssetGroupTagType...)). Consider using the same pattern for consistency.- SQLString: "type = 2" + SQLString: "type = " + strconv.Itoa(int(model.SelectorTypeCypher))Note: Verify the correct constant based on the test context.
cmd/api/src/api/v2/auth/auth_test.go (1)
2828-2828: Consider potential fragility in UUID string concatenation.While UUID.String() output is predictable and safe, the string concatenation approach creates tight coupling between tests and SQL string formatting. Consider if this approach could be made more maintainable.
-model.SQLFilter{SQLString: "user_id = '" + user.ID.String() + "'"} +// Consider using a helper function or constants for filter patternsAlso applies to: 3065-3065
cmd/api/src/api/v2/assetgrouptags_test.go (1)
131-131: Consider simplifying the enum-to-string conversion.The current approach
strconv.Itoa(int(model.AssetGroupTagTypeLabel))works correctly but could potentially be simplified if the enum types support direct string conversion. However, given this is part of a broader refactoring effort, the current approach is acceptable.Consider if this could be simplified to:
-SQLString: "type = " + strconv.Itoa(int(model.AssetGroupTagTypeLabel)), +SQLString: fmt.Sprintf("type = %d", model.AssetGroupTagTypeLabel),Though both approaches are functionally equivalent,
fmt.Sprintfmight be slightly more readable.Also applies to: 156-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmd/api/src/api/v2/agi_test.go(2 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(2 hunks)cmd/api/src/api/v2/audit_test.go(1 hunks)cmd/api/src/api/v2/auth/auth_test.go(7 hunks)cmd/api/src/api/v2/auth/sso_test.go(1 hunks)cmd/api/src/api/v2/fileingest_test.go(1 hunks)cmd/api/src/api/v2/model.go(1 hunks)cmd/api/src/database/assetgrouptags_test.go(4 hunks)cmd/api/src/database/sso_providers_test.go(2 hunks)cmd/api/src/model/audit.go(1 hunks)cmd/api/src/model/filter.go(2 hunks)cmd/api/src/model/filter_test.go(1 hunks)packages/go/cypher/models/pgsql/model.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml(1 hunks)packages/go/openapi/src/schemas/api.response.all-findings.yaml(1 hunks)packages/go/openapi/src/schemas/model.unified-finding.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- cmd/api/src/api/v2/model.go
- packages/go/openapi/src/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/go/cypher/models/pgsql/model.go
- cmd/api/src/model/audit.go
- packages/go/openapi/doc/openapi.json
- cmd/api/src/model/filter_test.go
- cmd/api/src/model/filter.go
🧰 Additional context used
🧬 Code Graph Analysis (5)
cmd/api/src/api/v2/audit_test.go (1)
cmd/api/src/model/filter.go (1)
SQLFilter(96-99)
cmd/api/src/api/v2/fileingest_test.go (3)
cmd/api/src/services/job/management.go (1)
GetAllIngestJobs(34-36)cmd/api/src/model/filter.go (1)
SQLFilter(96-99)cmd/api/src/model/jobs.go (1)
IngestJob(28-40)
cmd/api/src/api/v2/agi_test.go (1)
cmd/api/src/model/filter.go (1)
SQLFilter(96-99)
cmd/api/src/database/assetgrouptags_test.go (2)
cmd/api/src/model/filter.go (1)
SQLFilter(96-99)cmd/api/src/model/assetgrouptags.go (2)
AssetGroupTagTypeLabel(43-43)AssetGroupTagTypeTier(42-42)
cmd/api/src/api/v2/assetgrouptags_test.go (1)
cmd/api/src/model/assetgrouptags.go (2)
AssetGroupTagTypeLabel(43-43)AssetGroupTagTypeTier(42-42)
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/schemas/api.response.all-findings.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/schemas/model.unified-finding.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (15)
cmd/api/src/api/v2/audit_test.go (1)
224-224: SQL filter format updated consistently with broader refactoring.The change from parameterized query format to inline SQL is consistent with the architectural changes mentioned in the AI summary. The string value is properly quoted.
cmd/api/src/api/v2/fileingest_test.go (1)
103-103: SQL filter format consistently updated.The numeric value is correctly formatted without quotes, maintaining proper SQL syntax in the inline filter string.
cmd/api/src/api/v2/auth/sso_test.go (1)
188-188: String value with spaces properly quoted in SQL filter.The inline SQL format correctly handles the string value containing spaces with proper single-quote escaping.
cmd/api/src/api/v2/agi_test.go (2)
151-151: SQL filter format consistently updated.The string value is properly quoted, following the same pattern established across all modified test files in this refactoring.
1093-1093: Numeric value correctly formatted in inline SQL filter.The numeric ID value is properly formatted without quotes, maintaining correct SQL syntax in the updated filter format.
cmd/api/src/database/sso_providers_test.go (1)
296-298: Consistent with previous change but same maintainability concerns.This change follows the same pattern as line 242, maintaining consistency within the file while sharing the same trade-offs regarding maintainability and type safety.
cmd/api/src/database/assetgrouptags_test.go (3)
23-23: Import addition supports the SQL filter changes.The
strconvimport is appropriately added to support the string conversion approach used in the modified SQL filters.
255-255: String conversion approach maintains constant reference.Using
strconv.Itoa(int(model.AssetGroupTagTypeLabel))is preferable to hardcoding the value as it maintains the relationship to the type constant, though it's more verbose than the original parameterized approach.
273-273: Consistent pattern with line 255.This change follows the same string conversion pattern as line 255, maintaining consistency within the file and preserving the reference to the type constant.
cmd/api/src/api/v2/auth/auth_test.go (4)
425-425: LGTM - Simple literal filter.The hardcoded filter string is appropriate for this test scenario.
476-476: LGTM - Consistent with test data.The filter correctly matches the test data being used.
818-818: LGTM - Simple equality filter.The filter string correctly represents the expected query condition.
1265-1265: LGTM - Appropriate for test context.The filter matches the test scenario for user filtering.
cmd/api/src/api/v2/assetgrouptags_test.go (2)
131-131: LGTM: Test expectation correctly updated for new SQL filter format.The change from parameterized queries to direct string concatenation in the mock expectation aligns with the broader refactoring to PostgreSQL AST builder mentioned in the PR summary.
156-156: LGTM: Consistent with the SQL filter format change.This follows the same pattern as line 131, correctly converting the enum value to a string for direct SQL concatenation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/go/openapi/src/schemas/api.response.all-findings.yaml (1)
23-31: 🛠️ Refactor suggestionCorrect
$refpath and asset typing
Refinefindings.items.$refto reference the sibling schema (./model.unified-finding.yaml) and constrainfinding_assetsto Base64‐encoded strings by changing itsadditionalPropertiestotype: stringwithformat: byte.Proposed diff:
- items: - $ref: './../schemas/model.unified-finding.yaml' + items: + $ref: './model.unified-finding.yaml' - finding_assets: - type: object - description: Directory structure of Base64 encoded assets related to each finding organized by finding name. - additionalProperties: - type: object + finding_assets: + type: object + description: Directory structure of Base64 encoded assets related to each finding organized by finding name. + additionalProperties: + type: string + format: bytepackages/go/openapi/src/schemas/model.unified-finding.yaml (1)
73-76:⚠️ Potential issueFix invalid
target_propertiestype
The second variant declarestarget_propertiesastype: stringwithadditionalProperties, which is invalid. It should mirror the first variant as an object.Proposed diff:
- target_properties: - type: string - additionalProperties: - type: object + target_properties: + type: object + additionalProperties: + type: object
🧹 Nitpick comments (3)
cmd/api/src/model/filter_test.go (2)
26-31: Prefer typed operator constants over raw string literalsThe tests embed operator strings (
"eq","gt", …).
Importing and using themodel.*operator constants (e.g.model.Equals) gives compile-time safety, auto-completion, and flags accidental typos at build time.
This also keeps the test in sync with any future refactors that might rename the literals.
165-170: Fix minor typo in sub-test name
"aprox equals"→"approx equals"– keeps the test descriptions professional and searchable.packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
35-35: Normalize$refformat for schema reference
Drop the redundant./prefix in the$refpath for consistency. Use:$ref: '../schemas/api.response.all-findings.yaml'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmd/api/src/api/v2/agi_test.go(2 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(2 hunks)cmd/api/src/api/v2/audit_test.go(1 hunks)cmd/api/src/api/v2/auth/auth_test.go(7 hunks)cmd/api/src/api/v2/auth/sso_test.go(1 hunks)cmd/api/src/api/v2/fileingest_test.go(1 hunks)cmd/api/src/api/v2/model.go(1 hunks)cmd/api/src/database/assetgrouptags_test.go(4 hunks)cmd/api/src/database/sso_providers_test.go(2 hunks)cmd/api/src/model/filter.go(2 hunks)cmd/api/src/model/filter_test.go(1 hunks)packages/go/cypher/models/pgsql/model.go(1 hunks)packages/go/openapi/doc/openapi.json(5 hunks)packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml(1 hunks)packages/go/openapi/src/paths/custom-nodes.custom-nodes.name.yaml(1 hunks)packages/go/openapi/src/paths/custom-nodes.custom-nodes.yaml(1 hunks)packages/go/openapi/src/schemas/api.response.all-findings.yaml(1 hunks)packages/go/openapi/src/schemas/model.unified-finding.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/go/openapi/src/paths/custom-nodes.custom-nodes.name.yaml
- cmd/api/src/api/v2/model.go
- cmd/api/src/api/v2/fileingest_test.go
- cmd/api/src/api/v2/auth/sso_test.go
- packages/go/cypher/models/pgsql/model.go
- packages/go/openapi/src/paths/custom-nodes.custom-nodes.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/api/src/api/v2/audit_test.go
- cmd/api/src/api/v2/agi_test.go
- cmd/api/src/database/sso_providers_test.go
- cmd/api/src/api/v2/assetgrouptags_test.go
- cmd/api/src/database/assetgrouptags_test.go
- cmd/api/src/api/v2/auth/auth_test.go
- cmd/api/src/model/filter.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/api/src/model/filter_test.go (1)
cmd/api/src/model/filter.go (4)
Filters(446-446)SQLFilter(96-99)Filter(441-444)BuildSQLFilter(198-269)
🪛 Checkov (3.2.334)
packages/go/openapi/doc/openapi.json
[HIGH] 1-19167: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/schemas/api.response.all-findings.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
packages/go/openapi/src/schemas/model.unified-finding.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (5)
packages/go/openapi/src/paths/attack-paths.attack-paths-details.yaml (1)
31-35:✅ Verification successful
Verify response envelope follows project conventions
Your 200 response wraps theapi.response.all-findingsschema under adataproperty. Confirm that other endpoints use the same envelope pattern; if they reference schemas directly at the top level, adjust accordingly.You can search for existing patterns with:
🏁 Script executed:
rg "properties:\n data:" -n packages/go/openapi/src/pathsLength of output: 252
To get accurate results on where the
dataenvelope is used across all path specs, let’s enable multiline matching and re-run the search:
🏁 Script executed:
#!/bin/bash # Search for any schema wrapped under a `data` property in path definitions rg -U "properties:\s*data:" -n packages/go/openapi/src/pathsLength of output: 20010
Enveloped response confirmed—no changes needed
The200response inattack-paths.attack-paths-details.yamlwraps its payload under adataproperty, matching the pattern used across all other path specs inpackages/go/openapi/src/paths.packages/go/openapi/src/schemas/model.unified-finding.yaml (1)
39-42: Verifyenvironment_iddata type
environment_idis defined as an object with arbitrary properties, but IDs are usually strings. Confirm the intended schema or update to:environment_id: type: stringpackages/go/openapi/doc/openapi.json (3)
2894-2919: Wrap example nodes underdata
The example now correctly encapsulates the node list in thedataproperty, aligning with the updated schema that expects adataarray.
3064-3068: Consistent icon nesting in example
Theicondetails are now properly nested underconfig.icon, matching the updated schema structure for custom nodes.
14478-14526: Add/api/v2/attack-paths/detailsendpoint with comprehensive responses
The newPUToperation defines all standard response codes (200,400,401,403,429,500) and references theapi.response.all-findingsschema for the200response.
| // Run each test case and compare the output with the expected result | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| actualOutput, err := model.BuildSQLFilter(tc.input) | ||
|
|
||
| _, err := queryParameterFilterMap.BuildSQLFilter() | ||
| require.Contains(t, err.Error(), "invalid filter predicate") | ||
| } | ||
| if tc.wantErr { | ||
| if err == nil { | ||
| t.Errorf("expected error, got nil") | ||
| } | ||
|
|
||
| func TestModel_BuildSQLFilter_Success(t *testing.T) { | ||
| numericMin := model.QueryParameterFilter{ | ||
| Name: "filtercolumn1", | ||
| Operator: model.GreaterThan, | ||
| Value: "0", | ||
| IsStringData: false, | ||
| } | ||
|
|
||
| numericMax := model.QueryParameterFilter{ | ||
| Name: "filtercolumn2", | ||
| Operator: model.LessThan, | ||
| Value: "10", | ||
| IsStringData: false, | ||
| } | ||
|
|
||
| stringValue := model.QueryParameterFilter{ | ||
| Name: "filtercolumn3", | ||
| Operator: model.Equals, | ||
| Value: "stringValue", | ||
| IsStringData: true, | ||
| } | ||
|
|
||
| boolEquals := model.QueryParameterFilter{ | ||
| Name: "filtercolumn4", | ||
| Operator: model.Equals, | ||
| Value: "true", | ||
| IsStringData: false, | ||
| } | ||
|
|
||
| boolNotEquals := model.QueryParameterFilter{ | ||
| Name: "filtercolumn5", | ||
| Operator: model.NotEquals, | ||
| Value: "false", | ||
| IsStringData: false, | ||
| } | ||
|
|
||
| approximatelyEquals := model.QueryParameterFilter{ | ||
| Name: "filtercolumn6", | ||
| Operator: model.ApproximatelyEquals, | ||
| Value: "testing value", | ||
| IsStringData: true, | ||
| } | ||
|
|
||
| expectedResults := map[string]model.SQLFilter{ | ||
| "numericMin": {SQLString: fmt.Sprintf("%s > ?", numericMin.Name), Params: []any{numericMin.Value}}, | ||
| "numericMax": {SQLString: fmt.Sprintf("%s < ?", numericMax.Name), Params: []any{numericMax.Value}}, | ||
| "stringValue": {SQLString: fmt.Sprintf("%s = ?", stringValue.Name), Params: []any{stringValue.Value}}, | ||
| "boolEquals": {SQLString: fmt.Sprintf("%s = ?", boolEquals.Name), Params: []any{boolEquals.Value}}, | ||
| "boolNotEquals": {SQLString: fmt.Sprintf("%s <> ?", boolNotEquals.Name), Params: []any{boolNotEquals.Value}}, | ||
| "stringApproximatelyEquals": {SQLString: fmt.Sprintf("%s ILIKE ?", approximatelyEquals.Name), Params: []any{approximatelyEquals.Value}}, | ||
| } | ||
|
|
||
| queryParameterFilterMap := model.QueryParameterFilterMap{ | ||
| numericMax.Name: model.QueryParameterFilters{numericMin, numericMax}, | ||
| stringValue.Name: model.QueryParameterFilters{stringValue}, | ||
| boolEquals.Name: model.QueryParameterFilters{boolEquals}, | ||
| boolNotEquals.Name: model.QueryParameterFilters{boolNotEquals}, | ||
| approximatelyEquals.Name: model.QueryParameterFilters{approximatelyEquals}, | ||
| } | ||
| return | ||
| } | ||
|
|
||
| result, err := queryParameterFilterMap.BuildSQLFilter() | ||
| require.Nil(t, err) | ||
| if err != nil { | ||
| t.Errorf("unexpected error: %v", err) | ||
| return | ||
| } | ||
|
|
||
| for _, val := range expectedResults { | ||
| require.Contains(t, result.SQLString, val.SQLString) | ||
| require.EqualValues(t, result.Params, result.Params) | ||
| if actualOutput.SQLString != tc.output.SQLString { | ||
| t.Errorf("incorrect SQL string: got %q, want %q", actualOutput.SQLString, tc.output.SQLString) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make assertions case-insensitive & verify Params slice
BuildSQLFilter relies on the formatter used by the pgsql AST builder, which may emit SQL keywords in upper-case (LIKE, IS, IS NOT, …).
A strict, case-sensitive comparison risks brittle false negatives.
Additionally, asserting that Params is empty guards against accidental regression to parameterised queries.
@@
- if actualOutput.SQLString != tc.output.SQLString {
- t.Errorf("incorrect SQL string: got %q, want %q", actualOutput.SQLString, tc.output.SQLString)
- }
+ if !strings.EqualFold(actualOutput.SQLString, tc.output.SQLString) {
+ t.Errorf("incorrect SQL string (case-insensitive compare): got %q, want %q",
+ actualOutput.SQLString, tc.output.SQLString)
+ }
+
+ if len(actualOutput.Params) != 0 {
+ t.Errorf("expected no query parameters, got %d", len(actualOutput.Params))
+ }Remember to extend the import block:
-import (
- "testing"
- "github.com/specterops/bloodhound/src/model"
-)
+import (
+ "testing"
+ "strings"
+
+ "github.com/specterops/bloodhound/src/model"
+)🤖 Prompt for AI Agents
In cmd/api/src/model/filter_test.go lines 188 to 208, the test compares SQL
strings case-sensitively, which can cause false negatives due to uppercase
keywords from the pgsql AST builder. Modify the assertion to compare SQL strings
case-insensitively. Also, add an assertion to verify that the Params slice in
actualOutput is empty to prevent regressions involving parameterized queries.
Update the import block as needed to support case-insensitive comparison.
chore (api, database): refactor SQL filter creation to use the PG SQL AST in dawgs
Description
Support for adding a new Audit enumeration value for supporting export of all findings from BHE.
Motivation and Context
Resolves BED-5973
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes