Skip to content

fix: avoid double-encoding alert URLs - #6653

Merged
shahargl merged 1 commit into
keephq:mainfrom
aditya-786:fix/alert-url-idempotent-encoding
Jul 28, 2026
Merged

fix: avoid double-encoding alert URLs#6653
shahargl merged 1 commit into
keephq:mainfrom
aditya-786:fix/alert-url-idempotent-encoding

Conversation

@aditya-786

Copy link
Copy Markdown
Contributor

Closes #4737

📑 Description

Alert URL validation encoded the % in existing %HH sequences each time an alert DTO was reconstructed. It also encoded #, which broke hash-based URLs such as Wazuh agent links.

This change preserves valid percent escapes and fragments while encoding malformed percent signs exactly once. It adds regression coverage for encoded Unicode, hash fragments, raw unsafe characters, scheme-less URLs, raw Unicode paths, and malformed percent escapes, including reconstruction idempotence.

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

ℹ Additional Information

Validation completed:

  • pytest tests/test_alert_dto.py -k url_encoding_is_idempotent --noconftest -o addopts='' -q
  • pytest tests/test_alert_dto.py -k 'not test_alert_started_at' -o addopts='' -q
  • ruff check keep/api/models/alert.py tests/test_alert_dto.py
  • isort --check-only keep/api/models/alert.py tests/test_alert_dto.py
  • black --check tests/test_alert_dto.py

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. Bug Something isn't working labels Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.43%. Comparing base (98dbd7e) to head (9ef07de).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
keep/api/models/alert.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6653       +/-   ##
===========================================
+ Coverage   30.49%   46.43%   +15.93%     
===========================================
  Files         101      178       +77     
  Lines       11741    18694     +6953     
===========================================
+ Hits         3581     8681     +5100     
- Misses       8160    10013     +1853     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shahargl shahargl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 28, 2026
@shahargl
shahargl merged commit dabf34b into keephq:main Jul 28, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: The data in the url field of the alarm data is doubly encoded.

2 participants