Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[TB] Fix CodeQL scan findings #687

Open
aaronreed708 opened this issue Oct 24, 2023 · 5 comments
Open

[TB] Fix CodeQL scan findings #687

aaronreed708 opened this issue Oct 24, 2023 · 5 comments
Labels
bug Something isn't working theme builder app Theme Builder application

Comments

@aaronreed708
Copy link
Contributor

Problem/Concern

It looks like there are issues being reported by CodeQL in GitHub (https://github.com/finos/a11y-theme-builder/security/code-scanning) that we need to investigate and resolve. The high severity ones seem to suggest that we should be using rate-limiting middleware to prevent filesystem requests triggered by APIs to possibly cause a denial of service if they are triggered too frequently.

Maybe not a huge issue since Netlify is just serving our static urls so it won't affect those deployments, but may/will affect anyone deploying the code using Docker, etc.

Proposed Solution

@aaronreed708 aaronreed708 added bug Something isn't working theme builder app Theme Builder application labels Oct 24, 2023
@aaronreed708 aaronreed708 moved this to High Priority in ThemeBuilder Oct 26, 2023
@eliblurrtt
Copy link

eliblurrtt commented Nov 14, 2023

Hello @aaronreed708, Taking into consideration how this might affect UI/UX, I would recommend a dynamic rate limiter(IP based) middleware in express that only applies to requests that are not from the same origin. This approach will involve setting environment variables(Default values can be specified in main program). A Fixed Window Counter with Retries(Exponentially backed off) should be okay for the rate limiter. Also there will be a need to include a Retry-After header in response

@aaronreed708
Copy link
Contributor Author

Hi @eliblurrtt, your approach sounds good. Is this what you are looking to use? https://www.npmjs.com/package/express-rate-limit. I saw a couple of tutorials online showing how to use it. Or do you have a different one in mind? How do you propose to test it?

@aaronreed708
Copy link
Contributor Author

A contributor in today's call suggested that maybe this is better handled by the entities deploying theme builder to do the rate limiting.

@PaulaPaul
Copy link
Contributor

PaulaPaul commented Dec 7, 2023

Hello @aaronreed708 - I'd like to take a look at this but do not have access to this link: https://github.com/finos/a11y-theme-builder/security/code-scanning - can you help? Agree with the comment that rate limiting is best handled by the organization hosting the product(s) using theme builder, but we could note these recommendations and more in documentation (perhaps a Security Considerations section of the documentation). We can also address the security posture for Theme Builder including potential OpenSSF best practices badge: https://www.bestpractices.dev/en/criteria/0

Based on discussion at the project call, @PaulaPaul will open a separate issue for obtaining the OpenSSF best practices badge.

@aaronreed708
Copy link
Contributor Author

Hello @aaronreed708 - I'd like to take a look at this but do not have access to this link: https://github.com/finos/a11y-theme-builder/security/code-scanning - can you help?

@PaulaPaul @TheJuanAndOnly99 has asked us to use help@finos.org for access requests. Please try that avenue and let me know if that runs into an issue.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working theme builder app Theme Builder application
Projects
Status: High Priority
Development

No branches or pull requests

3 participants