Skip to content
This repository has been archived by the owner on Jun 4, 2020. It is now read-only.

Minor cleanup and bug fixes #11

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

remydagostino
Copy link

There are some pretty big security holes in this lib. I would have liked to have fixed them up but I found the code was deeply nested and hard to work with. I've made some minor fixes to the structure - just extracting methods and removing some dependencies on shared scopes. I'm not perfectly happy with the result but I do think it moves us somewhat toward a code base that can be more easily worked on.

I also found a couple of bugs while I was working, which I believe I have fixed.

  • Multiple requests over a short period to the same theme that has not yet been installed and downloaded causes rather erratic behaviour. This is due to a race condition where two "threads" are fighting to install the same theme at the same time. I've fixed this so that multiple calls to download the same theme are coalesced into a single one.
  • I've added another route to the express server to handle incoming requests to favicon.ico. The request is simply terminated. Previously this was being treated as a request to download the theme jsonresume-theme-favicon.ico.

Other than the clean-up and those bug fixes there should not be any material changes to the behaviour of the theme manager. Moving forward I think we should talk about a more major restructuring of the code as the current implementation is very fragile and insecure.

This is my first open source contribution. Please be kind if I have made any faux pas.

@thomasdavis
Copy link
Member

Yeah, those security issues are pretty high priority now that we are getting some traction.

Will look at the changes later tonight, sounds pretty good though.

Still overseas?

@remydagostino
Copy link
Author

Thanks. I'm back in Brisbane for at least another week, maybe two.

I'm not sure what the best way to handle the security issues would be. At this point though I think it might just be best to get them documented and clearly flag the unsafe functions so that some linux security wizard can come along and fix them. :)

@PeterDaveHello
Copy link
Member

any updates?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants