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

Escape visible projects object in current-projects.js to prevent Liquid injections #3333

Conversation

BeckettOBrien
Copy link
Member

Fixes #3257

What changes did you make and why did you make them ?

In assets/js/current-projects.js

  • Escaped the visible_projects object with JSON.parse(decodeURIComponent())
  • Rewrote lines 78-120 to work with the new visible_projects object
  • Replaced all uses of project.projectAreas with project['project-area'] to work with the new method of parsing projects
  • Changed line 89 to parse the project identification to an Integer to work with the new method of parsing projects
  • Sanitizing the visible_projects object prevents possible Liquid injection attacks as described in the Hack for LA Liquid Injection Vulnerabilities Audit

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Only changed code to sanitize inputs. No visual changes to the site.

@github-actions
Copy link

github-actions bot commented Jul 6, 2022

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b BeckettOBrien-escape-liquid-injection-3257 gh-pages
git pull https://github.com/BeckettOBrien/hfla-website.git escape-liquid-injection-3257

@github-actions github-actions bot added Feature: Refactor JS / Liquid Page is working fine - JS / Liquid needs changes to become consistent with other pages P-Feature: Projects page https://www.hackforla.org/projects/ role: back end/devOps Tasks for back-end developers size: 1pt Can be done in 4-6 hours Complexity: Small Take this type of issues after the successful merge of your second good first issue Status: Updated No blockers and update is ready for review labels Jul 6, 2022
@BeckettOBrien
Copy link
Member Author

One thing to consider before merging this is that some of the projects use HTML in their descriptions, which would no longer work after escaping liquid injections. For example, this is what the Undebate project card would look like after this issue is resolved:
image
I told @tamara-snyder about this in the original issue and I asked about it in the slack but I haven't gotten any responses yet so I thought I would create the PR and see if anyone has any thoughts.

@BeckettOBrien
Copy link
Member Author

Leaving a note here about all the projects that use HTML tags in their descriptions that would need to be changed under this PR:

@BeckettOBrien
Copy link
Member Author

Closing because too many projects use HTML in their descriptions for escaping Liquid injections to be worthwhile

@BeckettOBrien BeckettOBrien deleted the escape-liquid-injection-3257 branch July 7, 2022 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Small Take this type of issues after the successful merge of your second good first issue Feature: Refactor JS / Liquid Page is working fine - JS / Liquid needs changes to become consistent with other pages P-Feature: Projects page https://www.hackforla.org/projects/ role: back end/devOps Tasks for back-end developers size: 1pt Can be done in 4-6 hours Status: Updated No blockers and update is ready for review
Projects
Development

Successfully merging this pull request may close these issues.

Refactor code in current-projects.js to avoid Liquid injection vulnerability
1 participant