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

Fix switching tabs in the same module encodes the url #6775

Merged

Conversation

chantal-kelm
Copy link
Member

@chantal-kelm chantal-kelm commented Jun 14, 2024

Description

Fix the url encoding.

Issues Resolved

#6773

Evidence

Modules

Grabacion.de.pantalla.2024-06-14.a.la.s.12.03.14.p.m.mov

Rules

image

Endpoint summary > Configuration > Endpoint groups

Grabacion.de.pantalla.2024-06-19.a.la.s.8.56.21.p.m.mov

Health-check

Grabacion.de.pantalla.2024-06-19.a.la.s.8.58.41.p.m.mov

FIM Inventory tab table

Grabacion.de.pantalla.2024-06-19.a.la.s.9.02.09.p.m.mov

Test

Verify that in the parts shown in the video evidence the url is not encoded, update and remove the url parameters if necessary.

Check List

  • All tests pass
    • yarn test:jest
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@chantal-kelm chantal-kelm self-assigned this Jun 14, 2024
@chantal-kelm chantal-kelm linked an issue Jun 14, 2024 that may be closed by this pull request
Copy link
Member

@jbiset jbiset left a comment

Choose a reason for hiding this comment

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

CR: 🔴 (Comments added)

Test UI 🔴

Chrome 🟢

Evidence.webm

Firefox 🔴

image

@Desvelao
Copy link
Member

Some navigations are using the URLSearchParams constructor and composing the URL search parameters with the .toString method. According to the changes, it seems the fix is replace the usage of .toString() by a custom method to built the URL search parameters.

I guess this problem could appear in the remaining navigations that are using the URLSearchParamters.prototype.toString() method. In this case we should adapt too. For extension, we could create a custom constructor similar to URLSearchParameters with the implementation of URL search parameters build.

@JuanGarriuz JuanGarriuz self-requested a review June 18, 2024 14:17
@JuanGarriuz
Copy link
Member

JuanGarriuz commented Jun 18, 2024

Test 🟢

Modules

Chrome🟢
Grabacion.2024-06-11.123352.mp4

Rules

Chrome🟢

image

Endpoint Groups

Chrome🟢
Grabacion.2024-06-11.123352.mp4

Health Check

Chrome🟢
Grabacion.2024-06-11.123352.mp4

File Integrity Monitoring

Chrome🟢
Grabacion.2024-06-11.123352.mp4

@chantal-kelm
Copy link
Member Author

Some navigations are using the URLSearchParams constructor and composing the URL search parameters with the .toString method. According to the changes, it seems the fix is replace the usage of .toString() by a custom method to built the URL search parameters.

I guess this problem could appear in the remaining navigations that are using the URLSearchParamters.prototype.toString() method. In this case we should adapt too. For extension, we could create a custom constructor similar to URLSearchParameters with the implementation of URL search parameters build.

As mentioned in this comment it was decided to make a more global solution that attacks all files with the same problem.

I am working on it.

JuanGarriuz
JuanGarriuz previously approved these changes Jun 20, 2024
Copy link
Member

@jbiset jbiset left a comment

Choose a reason for hiding this comment

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

CR: 🟢

Test UI: 🟢

Modules 🟢

Chrome

Evidence1_Chrome.webm

Firefox

Evidence1_Firefox.webm

Rules 🟢

Chrome

image

Firefox

image

Endpoint summary > Configuration > Endpoint groups 🟢

Chrome

Evidence3_Chrome.webm

Firefox

Evidence3_Firefox.webm

Health-check 🟢

Chrome

Evidence4_Chrome.webm

Firefox

Evidence4_Firefox.webm

FIM Inventory tab table 🟢

Chrome

Evidence5_Chrome.webm

Firefox

Evidence5_Firefox.webm

Copy link
Member

@Desvelao Desvelao left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@JuanGarriuz JuanGarriuz left a comment

Choose a reason for hiding this comment

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

LGTM!!

@chantal-kelm chantal-kelm force-pushed the fix/6773-switching-tabs-in-the-same-module-encodes-the-URL branch from 3e5e064 to ea28924 Compare June 24, 2024 17:26
Copy link
Contributor

Wazuh Core plugin code coverage (Jest) test % values
Statements 45.96% ( 399 / 868 )
Branches 41.97% ( 157 / 374 )
Functions 44.01% ( 136 / 309 )
Lines 46.16% ( 397 / 860 )

Copy link
Contributor

Wazuh Check Updates plugin code coverage (Jest) test % values
Statements 76.44% ( 172 / 225 )
Branches 58.65% ( 61 / 104 )
Functions 61.7% ( 29 / 47 )
Lines 76.44% ( 172 / 225 )

Copy link
Contributor

Main plugin code coverage (Jest) test % values
Statements 12.2% ( 3993 / 32717 )
Branches 8.11% ( 1743 / 21486 )
Functions 11.7% ( 944 / 8063 )
Lines 12.39% ( 3890 / 31386 )

@lucianogorza lucianogorza merged commit c5c9d4c into 4.9.0 Jun 24, 2024
4 checks passed
@lucianogorza lucianogorza deleted the fix/6773-switching-tabs-in-the-same-module-encodes-the-URL branch June 24, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching tabs in the same module encodes the URL
5 participants