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 or address any potential security issues reported by LGTM in server.ts #875

Closed
tdonohue opened this issue Sep 24, 2020 · 2 comments · Fixed by #933
Closed

Fix or address any potential security issues reported by LGTM in server.ts #875

tdonohue opened this issue Sep 24, 2020 · 2 comments · Fixed by #933
Assignees
Labels
bug code task e/3 Estimate in hours
Milestone

Comments

@tdonohue
Copy link
Member

LGTM is currently reporting two potential issues in server.ts which it flags as "security" related. We should analyze these and determine either a fix, or possibly tell LGTM to ignore the issue (if it is determined to not be an issue for DSpace).

The issues in server.ts are listed as the top two issues in this list:

@tdonohue tdonohue added this to the 7.0beta5 milestone Sep 24, 2020
@tdonohue tdonohue added this to To Do in DSpace 7 Beta 5 via automation Sep 24, 2020
@artlowel
Copy link
Member

There are two issues:

1. There's no rate limiter on our express server, which makes it vulnerable to DOS attacks

We discussed this before. Since it is fairly easy to add a rate limiter in node. I propose we add it, and make it optional based on config in environment.ts, but enabled by default.

2. There's a line that disables certificate validation

That line is only executed when you enable SSL for the UI, but don't have a certificate, in which case certificate validation will be disabled and a self signed certificate will be used. I suggest we add a warning in the server output to make it more clear that the certificate can't be found and suppress this LGTM warning

Both of these will take an estimated 3h

@artlowel artlowel assigned tdonohue and unassigned artlowel Oct 21, 2020
@tdonohue
Copy link
Member Author

Thanks @artlowel, both your proposals sound reasonable, so I've added the 3h estimate to this ticket.

@tdonohue tdonohue added e/3 Estimate in hours and removed Estimate TBD labels Oct 21, 2020
@tdonohue tdonohue assigned artlowel and unassigned tdonohue Oct 21, 2020
@tdonohue tdonohue moved this from To Do to In Progress in DSpace 7 Beta 5 Oct 22, 2020
@artlowel artlowel linked a pull request Nov 6, 2020 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug code task e/3 Estimate in hours
Projects
No open projects
DSpace 7 Beta 5
  
In Progress
Development

Successfully merging a pull request may close this issue.

2 participants