arsmentis / coldfront Goto Github PK

Originally mentioned by @dmr-x in context of #4

Problem summary

The Add Publications feature (via search*) trusts the client's POST data from the search-results form. A slightly technically inclined user may change the data (publication title, author, etc.) in the form, and that altered data will simply be trusted and added to the database.

In the worst case, since the doi (or other id) forms part of a URL, this user would be able to change the URL used for the publication, potentially affecting all users in the project or others (admin, staff) that click on it.
doi.org for example provides a service that automatically redirects the browser to the publication's target page. A malicious user could create a doi pointing to a malicious page (or exploit an existing doi's target), thereby launching an attack; such as phishing, a XSS-vulnerable page, a browser exploit, etc.

This is admittedly unlikely, and adding a publication to a given project is already gated by user permission.

*NOTE: this is a pre-existing problem, and not a result of the feature in PR #4.

Problem technical

Buggy code snippet:

Simplest fix

0. Make use of commits* from PR #4 development.
1. Refactor search and search-results views to retrieve remote publication data twice: once for the search, and again after the client identifies which publication(s) to add.

Estimate to fix:
4 hours

*relevant commits:
371fa1c
f43afdf
7f2bf18
07d6b2f
9c8ae3f

Originally posted by @dmr-x in #4 (comment) - text modified for context

Problem summary

POST /publication/project/{project}/delete-publications/ fails for almost-duplicate publications.
The Delete Publications feature does not make use of guaranteed uniqueness in selecting publications to delete, but rather pulls them by Title and Year.

Additionally, the view showing these publications does not present enough detail to the user to know the difference between them.

Problem technical

Buggy code snippet:

objects.get() implicitly requires 1 result and raises an exception if more match.

Simplest fix

1. Use the publication's primary key instead of title/year to specify a publication.
2. In the view showing projects to delete, add a "detail view" (ideally mouseover) that presents additional details about the publication, thus allowing a user to determine the differences between almost-duplicate publications. NOTE: it may be hard for a typical user to distinguish the difference between almost-duplicate publications.

Estimate to fix:
6 hours

Originally mentioned by @dmr-x in context of #4

Problem summary

The Add Publications feature (via search*) makes use of python eval() directly on POST data. Use of eval() is often insecure, and use of it on user-controlled input is definitely insecure.

*NOTE: this is a pre-existing problem, and not a result of the feature in PR #4.

Problem technical

Buggy code snippet:

Simplest fix

0. Make use of commits* from PR #4 development.
1. Refactor search-results view to include publication data w/o use of eval(). json.dumps() and json.loads() are probably easiest.

Estimate to fix:
4 hours

*relevant commits:
9c8ae3f

Future suggestion

Use of static analysis tools, such as bandit, could easily identify similar issues during development.