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

Feature/843 revoke instance delegation2 #857

Merged
merged 9 commits into from
Oct 29, 2024
Merged

Feature/843 revoke instance delegation2 #857

merged 9 commits into from
Oct 29, 2024

Conversation

lovoll
Copy link
Contributor

@lovoll lovoll commented Oct 23, 2024 •
edited
Loading

Description

Related Issue(s)

Developer/Reviewer Checklist

  • Your code builds clean without any errors or warnings
  • No changes to config/appsettings or environment variables created in separate linked PR
  • Manual testing done (required)
  • Relevant integration test added (required for functional changes)
  • Relevant automated test added (required for functional changes)
  • All integration tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 23, 2024
View reviewed changes
src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs
string reasonPhrase = httpResponse.ReasonPhrase;
_logger.LogError(
"Writing of delegation policy at path: {policyPath} failed. Response Status Code:\n{status}. Response Reason Phrase:\n{reasonPhrase}",
policyPath,

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.

Copilot Autofix AI 3 months ago

To fix the problem, we need to sanitize the policyPath before logging it. Specifically, we should remove any newline characters from the policyPath to prevent log forging. This can be done using the Replace method to remove newline characters.

Suggested changeset 1
src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs b/src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs
--- a/src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs
+++ b/src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs
@@ -94,5 +94,6 @@
                         string reasonPhrase = httpResponse.ReasonPhrase;
+                        string sanitizedPolicyPath = policyPath.Replace("\n", "").Replace("\r", "");
                         _logger.LogError(
                             "Writing of delegation policy at path: {policyPath} failed. Response Status Code:\n{status}. Response Reason Phrase:\n{reasonPhrase}",
-                            policyPath,
+                            sanitizedPolicyPath,
                             status,
EOF
@@ -94,5 +94,6 @@
string reasonPhrase = httpResponse.ReasonPhrase;
string sanitizedPolicyPath = policyPath.Replace("\n", "").Replace("\r", "");
_logger.LogError(
"Writing of delegation policy at path: {policyPath} failed. Response Status Code:\n{status}. Response Reason Phrase:\n{reasonPhrase}",
policyPath,
sanitizedPolicyPath,
status,
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
src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 25, 2024
View reviewed changes
src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs Fixed Show fixed Hide fixed
src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs Fixed Show fixed Hide fixed
src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs
bool validPath = DelegationHelper.TryGetDelegationPolicyPathFromInstanceRule(rules, out string path);
if (validPath)
{
_logger.LogError(ex, "An exception occured while processing authorization rules for delegation on delegation policy path: {path}", path);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.

Copilot Autofix AI 3 months ago

To fix the problem, we need to sanitize the path variable before logging it. Since the log entries are plain text, we should remove any newline characters from the path to prevent log forging. This can be done using the Replace method to remove newline characters.

Suggested changeset 1
src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs b/src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs
--- a/src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs
+++ b/src/Altinn.AccessManagement.Core/Services/PolicyAdministrationPoint.cs
@@ -302,2 +302,3 @@
                 {
+                    string sanitizedPath = path.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
                     _logger.LogError(
@@ -305,3 +306,3 @@
                         "An exception occured while processing authorization rules for delegation on delegation policy path: {path}", 
-                        path);
+                        sanitizedPath);
                 }
@@ -344,3 +345,4 @@
                 {
-                    _logger.LogError(ex, "An exception occured while processing authorization rules for delegation on delegation policy path: {path}", path);
+                    string sanitizedPath = path.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
+                    _logger.LogError(ex, "An exception occured while processing authorization rules for delegation on delegation policy path: {path}", sanitizedPath);
                 }
EOF
@@ -302,2 +302,3 @@
{
string sanitizedPath = path.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
_logger.LogError(
@@ -305,3 +306,3 @@
"An exception occured while processing authorization rules for delegation on delegation policy path: {path}",
path);
sanitizedPath);
}
@@ -344,3 +345,4 @@
{
_logger.LogError(ex, "An exception occured while processing authorization rules for delegation on delegation policy path: {path}", path);
string sanitizedPath = path.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
_logger.LogError(ex, "An exception occured while processing authorization rules for delegation on delegation policy path: {path}", sanitizedPath);
}
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
@lovoll
Fixed some code smells
ef12f7c 
Added Authorization removed for local test
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
69.4% Coverage on New Code
1.6% Duplication on New Code

See analysis details on SonarCloud

@lovoll lovoll merged commit 5dbb8aa into main Oct 29, 2024
7 of 9 checks passed
@lovoll lovoll deleted the feature/843-RevokeInstanceDelegation2 branch October 29, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreasisnes andreasisnes andreasisnes left review comments

@jonkjetiloye jonkjetiloye jonkjetiloye approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lovoll @andreasisnes @jonkjetiloye