Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Kubero V3 refactoring #619

Draft
wants to merge 59 commits into
base: main
Choose a base branch
from
Draft

Kubero V3 refactoring #619

wants to merge 59 commits into from

Conversation

mms-gianni
Copy link
Member

@mms-gianni mms-gianni commented Jan 31, 2025 •
edited
Loading

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Template (non-breaking change which adds a template)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • I've built the image and tested it on a kubernetes cluster

Test Configuration:

  • Operator Version:
  • Kubernetes Version:
  • Kubero CLI Version (if applicable):

Checklist:

  • I removed unnecessary debug logs
  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I documented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@mms-gianni mms-gianni marked this pull request as draft January 31, 2025 13:34
@mms-gianni mms-gianni self-assigned this Feb 3, 2025
@mms-gianni mms-gianni added the in progress working on it label Feb 3, 2025
@mms-gianni mms-gianni linked an issue Feb 3, 2025 that may be closed by this pull request
Announcing Kubero v3 (Refactoring) #623
Open
@mms-gianni
Copy link
Member Author

Progress

image

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 13, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 13, 2025
View reviewed changes
server-refactored-v3/src/templates/templates.controller.ts
res.send(template);
} catch (err) {
this.logger.error(err);
res.status(500).send(err);

Check warning

Code scanning / CodeQL

Information exposure through a stack trace Medium

This information exposed to the user depends on
stack trace information
Loading
.

Copilot Autofix AI 1 day ago

To fix the problem, we need to ensure that stack traces and other sensitive information are not exposed to the end user. Instead, we should log the error details on the server and send a generic error message to the user. This can be achieved by modifying the catch block to log the error and send a generic message.

  • Modify the catch block to log the error using the existing logger.
  • Send a generic error message to the user instead of the error object.
Suggested changeset 1
server-refactored-v3/src/templates/templates.controller.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/src/templates/templates.controller.ts b/server-refactored-v3/src/templates/templates.controller.ts
--- a/server-refactored-v3/src/templates/templates.controller.ts
+++ b/server-refactored-v3/src/templates/templates.controller.ts
@@ -32,4 +32,4 @@
     } catch (err) {
-      this.logger.error(err);
-      res.status(500).send(err);
+      this.logger.error('An error occurred while fetching the template', err.stack);
+      res.status(500).send('An error occurred while processing your request.');
     }
EOF
@@ -32,4 +32,4 @@
} catch (err) {
this.logger.error(err);
res.status(500).send(err);
this.logger.error('An error occurred while fetching the template', err.stack);
res.status(500).send('An error occurred while processing your request.');
}
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
server-refactored-v3/src/templates/templates.controller.ts
res.send(template);
} catch (err) {
this.logger.error(err);
res.status(500).send(err);

Check warning

Code scanning / CodeQL

Exception text reinterpreted as HTML Medium

Exception text
Loading
is reinterpreted as HTML without escaping meta-characters.

Copilot Autofix AI 1 day ago

To fix the problem, we need to ensure that the error message is properly sanitized before being sent in the response. This can be achieved by using a library like he to escape any HTML special characters in the error message. This will prevent any potential XSS attacks by ensuring that the error message is treated as plain text.

  1. Install the he library for HTML entity encoding.
  2. Import the he library in the templates.controller.ts file.
  3. Use the he.encode function to sanitize the error message before sending it in the response.
Suggested changeset 2
server-refactored-v3/src/templates/templates.controller.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/src/templates/templates.controller.ts b/server-refactored-v3/src/templates/templates.controller.ts
--- a/server-refactored-v3/src/templates/templates.controller.ts
+++ b/server-refactored-v3/src/templates/templates.controller.ts
@@ -6,2 +6,3 @@
 import { OKDTO } from 'src/shared/dto/ok.dto';
+import * as he from 'he';
 
@@ -33,3 +34,3 @@
       this.logger.error(err);
-      res.status(500).send(err);
+      res.status(500).send(he.encode(err.toString()));
     }
EOF
@@ -6,2 +6,3 @@
import { OKDTO } from 'src/shared/dto/ok.dto';
import * as he from 'he';

@@ -33,3 +34,3 @@
this.logger.error(err);
res.status(500).send(err);
res.status(500).send(he.encode(err.toString()));
}
server-refactored-v3/package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/package.json b/server-refactored-v3/package.json
--- a/server-refactored-v3/package.json
+++ b/server-refactored-v3/package.json
@@ -55,3 +55,4 @@
     "sshpk": "^1.18.0",
-    "yaml": "^2.7.0"
+    "yaml": "^2.7.0",
+    "he": "^1.2.0"
   },
EOF
@@ -55,3 +55,4 @@
"sshpk": "^1.18.0",
"yaml": "^2.7.0"
"yaml": "^2.7.0",
"he": "^1.2.0"
},
This fix introduces these dependencies
Package Version Security advisories
he (npm) 1.2.0 None
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mms-gianni mms-gianni

Labels
in progress working on it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Announcing Kubero v3 (Refactoring)
1 participant
@mms-gianni