XL: App export/import rewrite

mrheinen commented 1 month ago

User description

Move from JSON export to YAML which makes it more user friendly to edit files by hand.

PR Type

Enhancement

Description

Enhanced database models to support YAML marshalling and UUID handling for Content, ContentRule, and Application.
Updated API server to handle YAML exports/imports and improved UUID management in content rules.
Implemented app import functionality in the API client, supporting both single file and directory imports.
Improved UI components across the application, adding UUID fields and enhancing list views.
Added new helper functions for string manipulation and date formatting.
Updated database schema to support UUID columns and removed obsolete fields.
Enhanced documentation for app import functionality and search keywords.
Implemented YAML support for app exports and imports, replacing the previous JSON format.
Improved error handling and validation for UUID fields throughout the application.

Changes walkthrough 📝

Relevant files

Enhancement

13 files

database.go `Database model updates for YAML support and UUID handling` pkg/database/database.go Added YammableBytes type for YAML marshalling of content data Updated struct fields to include YAML tags Added SetExternalUuid method to ExternalDataModel interface Modified Content, ContentRule, and Application structs with new fields and YAML tags Added InsertExternalModel method to DatabaseClient interface	+116/-63
server.go `API server updates for YAML export/import and UUID handling` pkg/api/server.go Updated HandleUpsertSingleContentRule to handle ContentUuid and AppUuid Modified ExportAppWithContentAndRule to use YAML for export Updated ImportAppWithContentAndRule to handle YAML imports and UUID validation	+175/-27
api_client.go `API client enhancements for app import functionality` cmd/api_client/api_client.go Added flags for app import functionality Implemented app import from file and directory Added GetFilesRecursivelyFromDir function for directory imports	+48/-2
generic_client.go `Generic API client updates for import functionality` pkg/api/generic_client.go Added Import method to ApiClient interface Implemented Import method for GenericApiClient Updated FakeApiClient to include Import method	+44/-1
ContentList.vue `Content list UI improvements` ui/src/components/container/ContentList.vue Updated content name display to use truncated version Imported helper functions for string truncation and date formatting	+6/-5
AppsList.vue `Apps list UI enhancements` ui/src/components/container/AppsList.vue Added action column with link to view rules for each app Imported dateToString helper function	+10/-4
RuleForm.vue `Rule form UI updates` ui/src/components/container/RuleForm.vue - Added UUID field to rule form - Improved layout of form fields	+29/-14
RulesList.vue `Rules list UI improvements` ui/src/components/container/RulesList.vue Updated link to view requests to use rule_uuid instead of rule_id Imported helper functions for string truncation and date formatting	+4/-12
AppForm.vue `App form and export updates` ui/src/components/container/AppForm.vue Added UUID field to app form Updated app export to use YAML format	+15/-3
ImportAppForm.vue `Import app form enhancements` ui/src/components/container/ImportAppForm.vue Added warning message for importing trusted files only Updated import request to include API key in headers	+4/-0
ContentForm.vue `Content form UI update` ui/src/components/container/ContentForm.vue - Added UUID field to content form	+14/-0
helpers.js `New helper functions for string manipulation and date formatting` ui/src/helpers.js - Added truncateString and dateToString helper functions	+10/-0
database.sql `Database schema updates for UUID support` config/database.sql Removed is_default column from content table Added rule_uuid column to request table Updated content_rule table structure (removed host, added app_uuid and content_uuid) Set default UUID generation for ext_uuid columns	+8/-7

Documentation

2 files

API_CLIENT.md `Documentation update for app import functionality` API_CLIENT.md Added documentation for importing previously exported apps Provided examples for importing single app and directory of apps	+38/-1
SEARCH_KEYWORDS.md `Search keywords documentation update` SEARCH_KEYWORDS.md Added rule_uuid to Request fields Updated Content and ContentRule field descriptions	+4/-1

Additional files (token-limit)

1 files

TeamCity-2023.11.3.yaml

...

rules/TeamCity-2023.11.3.yaml ...

+475/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions[bot] commented 1 month ago

Preparing PR description...

github-actions[bot] commented 1 month ago

Preparing review...

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 Security concerns

Sensitive information exposure:
The new app import functionality in pkg/api/server.go (lines 952-1068) may potentially expose sensitive information if not properly validated. Ensure that imported data is sanitized and does not contain any unintended sensitive information before storing or processing it.

⚡ Key issues to review

Data Handling
The new YammableBytes type and its YAML marshaling methods may need careful review to ensure proper encoding/decoding of binary data. Error Handling
The new app import functionality should be reviewed for proper error handling and validation of imported data. File Handling
The new recursive file import functionality should be reviewed for potential security risks when handling user-provided file paths.

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Input validation	✅ Add file type validation for app import ___ Consider adding input validation to ensure that only YAML files are selected for import. [ui/src/components/container/ImportAppForm.vue [7]](https://github.com/mrheinen/lophiid/pull/39/files#diff-e6fc9fc4d3942cc37125216ec18d70ef7cd858eb16ec6e107e1bce996a552333R7-R7) ```diff - + ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 9 Why: This suggestion adds an important security measure by restricting file types, which can prevent potential vulnerabilities from importing malicious files.	9
Input validation	Add validation for ContentID and AppID before fetching content and app ___ Consider adding validation for the `rb.ContentID` and `rb.AppID` fields before fetching the content and app. This can prevent unnecessary database queries if the IDs are invalid. [pkg/api/server.go [141-158]](https://github.com/mrheinen/lophiid/pull/39/files#diff-734f9c34b34af099d24da13054e167f7df72706dc733edf21c61c0cc1548224aR141-R158) ```diff if rb.ContentUuid == "" { + if rb.ContentID <= 0 { + a.sendStatus(w, "Invalid ContentID", ResultError, nil) + return + } content, err := a.dbc.GetContentByID(rb.ContentID) if err != nil { a.sendStatus(w, err.Error(), ResultError, nil) + return } - rb.ContentUuid = content.ExtUuid } // If we have no reference to the app UUID yet, do the same. if rb.AppUuid == "" { + if rb.AppID <= 0 { + a.sendStatus(w, "Invalid AppID", ResultError, nil) + return + } app, err := a.dbc.GetAppByID(rb.AppID) if err != nil { a.sendStatus(w, err.Error(), ResultError, nil) + return } - rb.AppUuid = app.ExtUuid } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Input validation before database queries can prevent unnecessary operations and improve error handling, enhancing overall robustness.	7
Data integrity	Use a transaction for the import operation to ensure atomicity ___ Consider using a transaction for the import operation in `ImportAppWithContentAndRule`. This ensures that all inserts and deletes are atomic, preventing partial imports in case of errors. [pkg/api/server.go [1071-1139]](https://github.com/mrheinen/lophiid/pull/39/files#diff-734f9c34b34af099d24da13054e167f7df72706dc733edf21c61c0cc1548224aR1071-R1139) ```diff -appModel, err := a.dbc.Insert(ae.App) +err := a.dbc.Transaction(func(tx sql.Tx) error { + appModel, err := a.dbc.Insert(ae.App) + if err != nil { + return err + } + + cm := make(map[string]database.Content) + for _, cnt := range ae.Contents { + cm[cnt.ExtUuid] = cnt + } + + for _, rule := range ae.Rules { + // ... (rule and content insertion logic) + } + + for _, rule := range existingRules { + if err := a.dbc.Delete(&rule); err != nil { + return err + } + } + + for _, cont := range existingContent { + if err := a.dbc.Delete(&cont); err != nil { + return err + } + } + + for _, app := range existingApps { + if err := a.dbc.Delete(&app); err != nil { + return err + } + } + + return nil +}) + if err != nil { a.sendStatus(w, err.Error(), ResultError, nil) return } -cm := make(map[string]database.Content) -for _, cnt := range ae.Contents { - cm[cnt.ExtUuid] = cnt -} - -for _, rule := range ae.Rules { - // ... (rule and content insertion logic) -} - -for _, rule := range existingRules { - if err := a.dbc.Delete(&rule); err != nil { - slog.Error("error deleting rule", slog.String("error", err.Error())) - } -} - -for _, cont := range existingContent { - if err := a.dbc.Delete(&cont); err != nil { - slog.Error("error deleting content", slog.String("error", err.Error())) - } -} - -for _, app := range existingApps { - if err := a.dbc.Delete(&app); err != nil { - slog.Error("error deleting app", slog.String("error", err.Error())) - } -} - ``` - [ ] Apply this suggestion* Suggestion importance[1-10]: 9 Why: Using a transaction ensures data consistency and integrity during complex operations involving multiple inserts and deletes.	9
Security	Enable alerting for a potentially critical security vulnerability ___ Consider setting the `alert` field to `true` for the rule with URI '/vendor/phpunit/phpunit/src/Util/PHP/eval-stdin.php' as it appears to be a potential security vulnerability that should trigger an alert. [rules/phpunit-4.8.28.yaml [29-44]](https://github.com/mrheinen/lophiid/pull/39/files#diff-b12ed1154c63dd763dbddc0a356b465328209e4b16bbb43beeef8dd09af98e61R29-R44) ```diff - id: 80 uri: /vendor/phpunit/phpunit/src/Util/PHP/eval-stdin.php method: POST port: 0 uri_matching: exact body_matching: none content_id: 99 app_id: 124 app_uuid: e57fc09b-7b0c-4fe2-a38f-74d24044f1c5 content_uuid: 994f7577-a249-44e2-a17c-4ffd2978aefb created_at: 2024-01-10T12:01:14.778938Z updated_at: 2024-09-13T16:11:40.030875Z - alert: false + alert: true ext_version: 1 ext_uuid: 6fcf2605-05c6-466a-a1f6-bf0d40c5994f requestpurpose: ATTACK ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: Enabling alerts for a potentially critical security vulnerability is crucial for timely detection and response to threats.	9
Error handling	Add error handling for UUID generation in the InsertExternalModel function ___ Consider adding error handling for the `SetExternalUuid` method calls in the `InsertExternalModel` function. If the `ExternalUuid` is empty and `uuid.NewString()` fails, it could lead to unexpected behavior. [pkg/database/database.go [556-558]](https://github.com/mrheinen/lophiid/pull/39/files#diff-1adb887d06a44193c36fc1c5708be385f3129cd59c2f2aa555faa065941ed877R556-R558) ```diff if dm.ExternalUuid() == "" { - dm.SetExternalUuid(uuid.NewString()) + newUUID, err := uuid.NewRandom() + if err != nil { + return dm, fmt.Errorf("failed to generate new UUID: %v", err) + } + dm.SetExternalUuid(newUUID.String()) } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Proper error handling for UUID generation is crucial for maintaining data integrity and preventing unexpected behavior.	8
	✅ Improve error handling in the directory traversal function ___ Suggestion Impact: The commit implemented enhanced error handling as suggested, providing more detailed error messages for directory traversal issues. code diff: ```diff if err != nil { - return err + return fmt.Errorf("error accessing path %q: %w", s, err) } if !d.IsDir() { retFiles = append(retFiles, s) } return nil }) - return retFiles, err + + if err != nil { + return retFiles, fmt.Errorf("error walking directory %q: %w", dir, err) + } + + return retFiles, nil ``` ___ Consider adding error handling for the `GetFilesRecursivelyFromDir` function to handle potential errors during directory traversal. [cmd/api_client/api_client.go [58-70]](https://github.com/mrheinen/lophiid/pull/39/files#diff-b1574db1d0a58fa08165598e3a92b7f057bad99f3543343a95828b9225dd95c6R58-R70) ```diff func GetFilesRecursivelyFromDir(dir string) ([]string, error) { var retFiles []string err := filepath.WalkDir(dir, func(s string, d fs.DirEntry, err error) error { if err != nil { - return err + return fmt.Errorf("error accessing path %q: %w", s, err) } if !d.IsDir() { retFiles = append(retFiles, s) } return nil }) - return retFiles, err + if err != nil { + return nil, fmt.Errorf("error walking directory %q: %w", dir, err) + } + return retFiles, nil } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Enhanced error handling provides more detailed information about potential issues, aiding in debugging and improving overall robustness.	8
	Improve error message specificity in the Import method ___ Consider using a more specific error message in the `Import` method when the API does not support import for a particular type. [pkg/api/generic_client.go [210-215]](https://github.com/mrheinen/lophiid/pull/39/files#diff-86e55a1fed585a7ea411843626b016f0b710653997dad82e66885fb40deef5f9R210-R215) ```diff switch any(check).(type) { case database.Application: slog.Debug("Importing application") default: - return fmt.Errorf("api does not support import: %s", a.apiLocation) + return fmt.Errorf("import not supported for API endpoint: %s", a.apiLocation) } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion slightly improves the error message clarity, which can be helpful for debugging, but it's not a critical change.	6
Best practice	Add a foreign key constraint to maintain referential integrity between tables ___ Consider adding a foreign key constraint for the `rule_id` column in the `request` table to reference the `id` column in the `content_rule` table to maintain referential integrity. [config/database.sql [58-62]](https://github.com/mrheinen/lophiid/pull/39/files#diff-002629893f0a5eb6f8e9a6d036047c92784e918c0afc132f6992827aae6121bcR58-R62) ```diff CREATE TABLE request ( id SERIAL PRIMARY KEY, created_at TIMESTAMP NOT NULL DEFAULT (timezone('utc', now())), base_hash VARCHAR(64) DEFAULT '', content_id INT, - rule_id INT -+ rule_uuid VARCHAR(36) default '', + rule_id INT, + rule_uuid VARCHAR(36) default '', + CONSTRAINT fk_rule_id FOREIGN KEY(rule_id) REFERENCES content_rule(id) ); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding a foreign key constraint improves data integrity and prevents orphaned records, which is a database best practice.	8
Best practice	Refactor the test function to use a table-driven test approach ___ Consider using a table-driven test approach for the `TestImportAppOk` function to improve test readability and maintainability. [pkg/api/server_test.go [515-521]](https://github.com/mrheinen/lophiid/pull/39/files#diff-c1b7ac336704ec14ba929be928d0348b2817bd747a0056fdb8a437903486435dR515-R521) ```diff func TestImportAppOk(t testing.T) { - - for _, test := range []struct { - description string + tests := []struct { + name string expectedStatus string appExport AppExport }{ + // Test cases here + } + for _, tt := range tests { + t.Run(tt.name, func(t testing.T) { + // Test logic here + }) + } + ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves test readability and maintainability, which is beneficial for long-term code quality.	7
Enhancement	Adjust the requestpurpose value to more accurately reflect the nature of the request ___ Consider using a more specific `requestpurpose` value for rules that are clearly intended for attacks or reconnaissance. For example, the rule with URI '/hax?jsp=/app/rest/debug/jvm/systemProperties;.jsp' could have `requestpurpose:` `ATTACK` instead of `RECON`. [rules/TeamCity-2023.11.3.yaml [13-29]](https://github.com/mrheinen/lophiid/pull/39/files#diff-3a7ad09593c538a216e9856a43ed1e4273dbd50cb1aec6e0c023320f5adfa5adR13-R29) ```diff +- id: 250 + uri: /hax?jsp=/app/rest/debug/jvm/systemProperties;.jsp + method: GET + port: 0 + uri_matching: exact + body_matching: none + content_id: 255 + app_id: 103 + app_uuid: 6cbb2aae-92f8-4cec-a493-9885d2c0aa98 + content_uuid: 3c0c972f-fd28-4984-ab77-0552bd767be0 + created_at: 2024-07-01T14:36:00.371465Z + updated_at: 2024-08-31T16:18:03.700405Z + alert: false + ext_version: 1 + ext_uuid: 98a32d7d-98c2-4d3c-a2f7-fd1080732f68 + requestpurpose: ATTACK - ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: This suggestion improves the accuracy of rule classification, which can enhance security monitoring and response.	7
Performance	Use a pointer receiver for the MarshalYAML method to improve performance ___ Consider using a pointer receiver for the `MarshalYAML` method of `YammableBytes`. This can improve performance by avoiding unnecessary copying of the slice. [pkg/database/database.go [67-69]](https://github.com/mrheinen/lophiid/pull/39/files#diff-1adb887d06a44193c36fc1c5708be385f3129cd59c2f2aa555faa065941ed877R67-R69) ```diff -func (yb YammableBytes) MarshalYAML() (interface{}, error) { - return base64.StdEncoding.EncodeToString(yb), nil +func (yb YammableBytes) MarshalYAML() (interface{}, error) { + return base64.StdEncoding.EncodeToString(yb), nil } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: Using a pointer receiver can improve performance by avoiding unnecessary copying, but the impact may be minimal for small slices.	6
Documentation	Add explanations for command-line flags to improve documentation clarity ___ Consider adding a brief explanation of the `-app-import` and `-app-import-file` / `-app-import-dir` flags in the documentation to provide more context for users. [API_CLIENT.md [68-75]](https://github.com/mrheinen/lophiid/pull/39/files#diff-4f9a453cabb33b282be95eade162e7f263e7262761aa00c7d7af26c67c372e7dR68-R75) ```diff +# Import a single app $ bazel run //cmd/api_client:api_client -- \ -api-server http://127.0.0.1:8088 \ -api-key ca357d9a-b98f-429c-83d7-e8088f69ba2f \ -app-import \ -app-import-file ./rules/Spring\ Framework-5.2.20.yaml + +# The -app-import flag enables app import mode +# The -app-import-file flag specifies the YAML file to import ... time=2024-09-13T11:51:56.090-04:00 level=INFO msg="Imported app" app="rules/Spring Framework-5.2.20.yaml" ``` Suggestion importance[1-10]: 6 Why: This suggestion enhances documentation clarity, making it easier for users to understand and use the command-line flags correctly.	6

mrheinen / lophiid