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: Osm username, add escaping where necessary #1158

Merged
merged 11 commits into from
Oct 19, 2023

Conversation

RitaDee
Copy link
Contributor

@RitaDee RitaDee commented Oct 18, 2023

Description

This PR focuses on improving the codebase in terms of security and consistency by making changes to how text content is inserted into various elements within the application. Specifically, I have replaced the use of .html() with .text() for setting text content, ensuring safer content insertion and aligning with best practices.

Fixes #1124

Screenshot:

image

Test Screenshot:

image

@Bonkles
Copy link
Contributor

Bonkles commented Oct 18, 2023

Hi @RitaDee! This is excellent, thank you for your work on this.

If possible, can you do some testing of these changes and show some screenshots of that testing, highlighting the areas where the username are displayed? This is good practice to show any github repo maintainer that your code works and hasn't broken anything.

@Bonkles
Copy link
Contributor

Bonkles commented Oct 18, 2023

One of the issues with our current codebase is that any string lifted from our core.yaml file (i.e. anything with a t() invocation or a tHtml() could be using inline to the string for formatting.

If the code modifications ONLY cover osm username rendering, then this should be good.

@Bonkles
Copy link
Contributor

Bonkles commented Oct 18, 2023

Also, I approved this PR to have our automated test workflow run some unit tests (results above). It appears that the node 16.14 build failed- but this is not due to any changes made in this PR, and is likely due to a flaky test. I do not consider that test failure a blocker by any means. :)

@RitaDee
Copy link
Contributor Author

RitaDee commented Oct 18, 2023

Also, I approved this PR to have our automated test workflow run some unit tests (results above). It appears that the node 16.14 build failed- but this is not due to any changes made in this PR, and is likely due to a flaky test. I do not consider that test failure a blocker by any means. :)

Thank you, @Bonkles, for the review. I will write the test and attach the screenshots.

@RitaDee
Copy link
Contributor Author

RitaDee commented Oct 18, 2023

Also, I approved this PR to have our automated test workflow run some unit tests (results above). It appears that the node 16.14 build failed- but this is not due to any changes made in this PR, and is likely due to a flaky test. I do not consider that test failure a blocker by any means. :)

Noted.

@Bonkles
Copy link
Contributor

Bonkles commented Oct 18, 2023

Thank you, @Bonkles, for the review. I will write the test and attach the screenshots.

I ran a very quick test of your branch and saw one issue: the contributors.js code is causing the following to happen in the bottom bar of Rapid:
image

@bhousel
Copy link
Contributor

bhousel commented Oct 18, 2023

Yeah sorry - some context on this is that we (Rapid) pulled in a bad regression from the iD project a while ago before the code diverged, and now there are places all around the Rapid code where the string sanitization has been removed. Every usage of .html really needs to be checked carefully because there is a lot of code that is expecting to pass span or other html tags through it.

some related issues:
openstreetmap/iD#8813
openstreetmap/iD#7998
openstreetmap/iD#8817

@RitaDee
Copy link
Contributor Author

RitaDee commented Oct 18, 2023

One of the issues with our current codebase is that any string lifted from our core.yaml file (i.e. anything with a t() invocation or a tHtml() could be using inline to the string for formatting.

If the code modifications ONLY cover osm username rendering, then this should be good.

I am not so clear on this. Do I revert the changes made on the file?

Thank you, @Bonkles, for the review. I will write the test and attach the screenshots.

I ran a very quick test of your branch and saw one issue: the contributors.js code is causing the following to happen in the bottom bar of Rapid: image

I have made an adjustment to this. This is what it looks like now:

image

@RitaDee
Copy link
Contributor Author

RitaDee commented Oct 18, 2023

Yeah sorry - some context on this is that we (Rapid) pulled in a bad regression from the iD project a while ago before the code diverged, and now there are places all around the Rapid code where the string sanitization has been removed. Every usage of .html really needs to be checked carefully because there is a lot of code that is expecting to pass span or other html tags through it.

some related issues: openstreetmap/iD#8813 openstreetmap/iD#7998 openstreetmap/iD#8817

I will check this out. Thank you

@RitaDee
Copy link
Contributor Author

RitaDee commented Oct 18, 2023

Hi @RitaDee! This is excellent, thank you for your work on this.

If possible, can you do some testing of these changes and show some screenshots of that testing, highlighting the areas where the username are displayed? This is good practice to show any github repo maintainer that your code works and hasn't broken anything.

Here is the screenshot of the test:

image

@RitaDee
Copy link
Contributor Author

RitaDee commented Oct 19, 2023

@Bonkles, this is ready for review.

Copy link
Contributor

@Bonkles Bonkles left a comment

Choose a reason for hiding this comment

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

Tested these changes, looks good!

@Bonkles Bonkles merged commit 374debc into facebook:main Oct 19, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OSM username isn't escaped properly
4 participants