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.

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

chore: Add HTTPs restriction for URLs on Attachments, ApiActions, GuiActions #1222

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

oskogstad
Copy link
Collaborator

@oskogstad oskogstad commented Oct 3, 2024

  • Adds HTTPS restrictions for URLs on Attachments, ApiActions, and GuiActions
  • Removes Markdig dependency and markdown validation
  • Removed FluentValidationLocalizationDtoExtensions, moved these functions to the string extensions

#1227

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new validation methods for HTTPS URLs and valid HTML content in various DTOs.
    • Enhanced the DialogGenerator to ensure all generated URIs utilize the HTTPS scheme.
  • Bug Fixes

    • Removed outdated validation methods for HTML and Markdown content, streamlining validation logic.
  • Refactor

    • Improved the structure of validation logic across multiple validators for clarity and maintainability.
  • Chores

    • Removed the package reference for Markdig.Signed to reduce dependencies.

@oskogstad oskogstad requested a review from a team as a code owner October 3, 2024 11:30
Copy link
Contributor

coderabbitai bot commented Oct 3, 2024

Warning

Rate limit exceeded

@oskogstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 19 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Files that changed from the base of the PR and between 1309c2b and c31e6c2.

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request implements significant changes to the validation logic within the application. It removes the FluentValidationLocalizationDtoExtensions class, which provided methods for validating HTML and Markdown content. New methods are introduced in the FluentValidationStringExtensions class to validate HTTPS URLs and HTML content. The project file also removes the Markdig.Signed package reference. Various validators are updated to use the new HTTPS URL validation methods, and the DialogGenerator class is modified to generate URIs with the HTTPS scheme.

Changes

File Change Summary
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/FluentValidation/FluentValidationLocalizationDtoExtensions.cs Removed class providing HTML and Markdown validation methods.
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/FluentValidation/FluentValidationStringExtensions.cs Added methods for validating HTTPS URLs and HTML content; updated existing URI validation method.
src/Digdir.Domain.Dialogporten.Application/Digdir.Domain.Dialogporten.Application.csproj Removed package reference for Markdig.Signed.
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs Refactored validation logic to use conditional rules; removed HTML and Markdown validations.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs Updated URL validation from IsValidUri() to IsValidHttpsUrl() across multiple validators.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs Updated URL validation from IsValidUri() to IsValidHttpsUrl() in several validators.
src/Digdir.Tool.Dialogporten.GenerateFakeData/DialogGenerator.cs Updated methods to generate URIs with HTTPS scheme for various entities.

Possibly related PRs

Suggested reviewers

  • MagnusSandgren
  • elsand

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@oskogstad oskogstad changed the title chore: chore: Add HTTPs restriction for URLs on Attachments, ApiActions, GuiActions Oct 3, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 6e7dfe4 and fb8e601.

📒 Files selected for processing (7)
  • src/Digdir.Domain.Dialogporten.Application/Common/Extensions/FluentValidation/FluentValidationLocalizationDtoExtensions.cs (0 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Common/Extensions/FluentValidation/FluentValidationStringExtensions.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Digdir.Domain.Dialogporten.Application.csproj (0 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (3 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (4 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs (4 hunks)
  • src/Digdir.Tool.Dialogporten.GenerateFakeData/DialogGenerator.cs (4 hunks)
💤 Files with no reviewable changes (2)
  • src/Digdir.Domain.Dialogporten.Application/Common/Extensions/FluentValidation/FluentValidationLocalizationDtoExtensions.cs
  • src/Digdir.Domain.Dialogporten.Application/Digdir.Domain.Dialogporten.Application.csproj
🔇 Additional comments (20)
src/Digdir.Tool.Dialogporten.GenerateFakeData/DialogGenerator.cs (6)

265-265: LGTM: HTTPS scheme enforced for ExtendedType URI

The change ensures that the generated ExtendedType URI uses the HTTPS scheme, which aligns with the PR objective of adding HTTPS restrictions. This improvement enhances security by ensuring only secure URLs are generated for dialog activities.


283-289: LGTM: HTTPS scheme enforced for all API action endpoint URLs

The changes ensure that the Url, RequestSchema, ResponseSchema, and DocumentationUrl properties of the API action endpoints use the HTTPS scheme. This aligns with the PR objective and enhances security by guaranteeing that only secure URLs are generated for these critical properties.


295-295: LGTM: HTTPS scheme enforced for process URI

The change ensures that the generated process URI uses the HTTPS scheme, which is in line with the PR objective of adding HTTPS restrictions. This improvement enhances security by guaranteeing that only secure URLs are generated for process URIs.


320-320: LGTM: HTTPS scheme enforced for GUI action URL

The change ensures that the generated URL for dialog GUI actions uses the HTTPS scheme, which aligns with the PR objective of adding HTTPS restrictions. This improvement enhances security by guaranteeing that only secure URLs are generated for GUI actions.


339-339: LGTM: HTTPS scheme enforced for attachment URL

The change ensures that the generated URL for dialog attachments uses the HTTPS scheme, which is in line with the PR objective of adding HTTPS restrictions. This improvement enhances security by guaranteeing that only secure URLs are generated for attachment URLs.


Line range hint 1-424: Overall assessment: Consistent HTTPS enforcement implemented successfully

The changes in this file consistently enforce the use of HTTPS for all generated URLs, including those for dialog activities, API action endpoints, process URIs, GUI actions, and attachment URLs. These modifications align perfectly with the PR objective of adding HTTPS restrictions for URLs on Attachments, ApiActions, and GuiActions.

The implementation is consistent across all relevant parts of the DialogGenerator class, which enhances the overall security of the generated fake data. This change is a valuable improvement to the codebase.

src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs (5)

131-131: Excellent security enhancement for transmission attachment URLs

The change from IsValidUri() to IsValidHttpsUrl() enforces HTTPS for transmission attachment URLs. This aligns perfectly with the PR objective and significantly enhances security by ensuring only secure URLs are used for attachments.


276-276: Consistent security improvement for dialog attachment URLs

The change from IsValidUri() to IsValidHttpsUrl() for dialog attachment URLs is consistent with the previous modification for transmission attachments. This change further reinforces the PR's objective of enhancing security by enforcing HTTPS across all attachment URLs.


303-303: Security enhancement completed for GUI action URLs

The change from IsValidUri() to IsValidHttpsUrl() for GUI action URLs completes the PR objective of adding HTTPS restrictions for Attachments, ApiActions, and GuiActions. This consistent approach across all URL validations significantly enhances the overall security of the system.


348-348: Consistent security enhancement for API action endpoint URLs

The change from IsValidUri() to IsValidHttpsUrl() for API action endpoint URLs is consistent with the previous modifications and further reinforces the PR's objective. This change ensures that HTTPS is enforced across all types of URLs in the system, including Attachments, GuiActions, and ApiActions, significantly enhancing the overall security posture.


Line range hint 131-348: Summary: Comprehensive HTTPS enforcement across all URL validators

The changes in this file consistently replace IsValidUri() with IsValidHttpsUrl() across all URL validators, including:

  1. Transmission attachment URLs
  2. Dialog attachment URLs
  3. GUI action URLs
  4. API action endpoint URLs

These modifications effectively implement the PR objective of adding HTTPS restrictions for Attachments, ApiActions, and GuiActions. This comprehensive approach significantly enhances the security of the system by ensuring that only secure HTTPS URLs are used throughout the application.

The consistency of these changes across different validators is commendable and demonstrates a thorough implementation of the security enhancement.

src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (5)

267-267: Improved security by enforcing HTTPS URLs

The change from IsValidUri() to IsValidHttpsUrl() enhances security by ensuring that only HTTPS URLs are accepted for attachments. This aligns with best practices for secure communication.


294-294: Consistent HTTPS enforcement for transmission attachments

This change mirrors the previous modification, ensuring HTTPS URLs for transmission attachments as well. This consistency in URL validation across different parts of the application is commendable.


321-321: Enhanced security for GUI action URLs

The implementation of IsValidHttpsUrl() for GUI actions ensures that all user-facing links adhere to secure HTTPS protocol. This is particularly important for GUI elements to maintain end-to-end security.


Line range hint 267-364: Summary: Comprehensive HTTPS enforcement across all URL validations

The changes in this file consistently replace IsValidUri() with IsValidHttpsUrl() across multiple validators, including those for attachments, GUI actions, and API endpoints. This systematic approach significantly enhances the security posture of the application by ensuring that all URLs adhere to the HTTPS protocol.

These modifications align perfectly with the PR objectives and represent a substantial improvement in the application's security. The consistent implementation across different components of the system is commendable and helps maintain a uniform security standard.

Great job on improving the overall security of the application!


364-364: Secured API action endpoints with HTTPS requirement

The change to IsValidHttpsUrl() for API action endpoints completes the consistent application of HTTPS across all URL validations in this file. This is a significant security enhancement.

However, it's crucial to verify that all existing API endpoints support HTTPS to prevent any potential issues with existing integrations.

To ensure all API endpoints support HTTPS, run the following script:

✅ Verification successful

Further Verification Needed for HTTPS Enforcement

To ensure that HTTPS is consistently enforced across all API endpoints, please run the following script to verify HTTPS configurations in the application settings and middleware:


HTTPS Enforcement Verified and Consistent

The application employs UseHttpsRedirection() in the following startup files to ensure all API endpoints enforce HTTPS:

  • src/Digdir.Domain.Dialogporten.Service/Program.cs
  • src/Digdir.Domain.Dialogporten.WebApi/Program.cs
  • src/Digdir.Domain.Dialogporten.ChangeDataCapture/Program.cs

Additionally, no non-HTTPS (http://) URLs were found in the codebase, confirming that HTTPS is consistently enforced across all API endpoints.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for all API endpoint definitions and verify HTTPS support

# Test: Look for API endpoint definitions
rg --type csharp -i 'api.*endpoint'

# Test: Check for any HTTP (non-HTTPS) URLs in the codebase
rg --type csharp 'http://'

Length of output: 35562


Script:

#!/bin/bash
# Description: Verify HTTPS enforcement in API configurations and middleware

# Check for HTTPS redirection in startup or configuration files
rg --type csharp 'UseHttpsRedirection\(\)' src/

# Check for HTTPS settings in configuration files
rg --type json '"https":\s*true' src/
rg --type yaml 'https:\s*true' src/

Length of output: 377

src/Digdir.Domain.Dialogporten.Application/Common/Extensions/FluentValidation/FluentValidationStringExtensions.cs (2)

29-34: Validate handling of Uri inputs in IsValidHttpsUrl

The method assumes that Uri objects are absolute. Ensure that any Uri inputs passed to this method are indeed absolute and that null handling is appropriate.

Run the following script to identify where IsValidHttpsUrl is used with Uri objects:

#!/bin/bash
# Description: Find usages of IsValidHttpsUrl with Uri inputs.

# Test: Search for calls to IsValidHttpsUrl with Uri parameters.
# Expect: Verify that Uri inputs are absolute.
rg --type cs --no-heading --line-number "IsValidHttpsUrl<.*?, Uri\?>"

22-27: Ensure null handling in IsValidHttpsUrl

The method correctly validates HTTPS URLs. However, consider whether null values are acceptable inputs. If null should be considered invalid, adjust the condition accordingly.

Run the following script to check for usages where null inputs are handled:

✅ Verification successful

Null Handling in IsValidHttpsUrl Confirmed

The IsValidHttpsUrl method appropriately handles null values as per its current implementation. No additional usages were found that contradict this behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find usages of IsValidHttpsUrl to verify null handling

# Test: Search for calls to IsValidHttpsUrl in the codebase.
# Expect: Review how null inputs are managed.
rg --type cs --no-heading --line-number "IsValidHttpsUrl" 

Length of output: 294

src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (2)

43-53: Validation logic correctly enforces HTTPS URLs for embeddable media types

The added conditional validation ensures that when the media type is embeddable, each value is validated as a valid HTTPS URL using IsValidHttpsUrl(). This aligns with the PR objective to enhance security by restricting URLs to HTTPS.


68-78: Validation logic correctly enforces HTTPS URLs for embeddable media types

Similarly to the previous constructor, this conditional validation ensures that embeddable media types have values that are valid HTTPS URLs, enhancing security as intended.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between fb8e601 and 1309c2b.

📒 Files selected for processing (1)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (2 hunks)
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (2)

30-40: Refactor duplicated validation logic into a private method

The validation logic within the When clauses from lines 30-40 and 55-65 is duplicated in both constructors. To improve maintainability and reduce code duplication, consider extracting this common logic into a private method or helper function.

Also applies to: 55-65


54-65: ⚠️ Potential issue

Ensure safe usage of user when accessing principal

In the GetAllowedMediaTypes method, when user is provided, you call user.GetPrincipal(). Ensure that the user object is not null to prevent potential NullReferenceException. If user can be null, include a check or consider using the null-conditional operator.

Apply this diff to add a null check:

 if (user == null)
 {
     return contentType.AllowedMediaTypes;
 }

-var allowHtmlSupport = user.GetPrincipal().HasScope(Constants.LegacyHtmlScope);
+var allowHtmlSupport = user?.GetPrincipal().HasScope(Constants.LegacyHtmlScope) ?? false;

Run the following script to identify any instances where ContentValueDtoValidator is instantiated with a potentially null user:

Copy link

sonarcloud bot commented Oct 4, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
3.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

1 participant