possible security issue with note editing

[btbonval]

It appears as though the only security in place is one check whether the note is editable and one check about who the user is to show the "Edit this Note" button.

I don't see any additional security with the form POST or anywhere else. If I'm reading the code correctly, it should be easy to bypass the security on the "Edit this Note" button.

Security should be baked into the model.

• https://github.com/FinalsClub/karmaworld/blob/note-editing-merge-more/karmaworld/templates/notes/note_base.html#L60-L86
• https://github.com/FinalsClub/karmaworld/blob/note-editing-merge-more/karmaworld/templates/notes/note_base.html#L183-L185
• https://github.com/FinalsClub/karmaworld/blob/note-editing-merge-more/karmaworld/apps/notes/forms.py#L16-L31

[btbonval]

It looks like we're going to fast track note editing to production for note import reasons. This ticket now has priority. Security bugs should be fixed before they are pushed to production! :)

[btbonval]

I thought this would be the place to intercept a Note POST, but it was not. https://github.com/FinalsClub/karmaworld/blob/note-editing-merge-more-ocw/karmaworld/apps/notes/views.py#L41-L42

I was able to edit a note as anonymous without triggering the protection I put in place before NoteForm(request.POST) is called.

It'd be great if I could intercept the POST saves the Note. I thought that was where to do it.

[btbonval]

Ah, that helper is just used for key terms and tags. I need to add a form_valid() to the NoteView(UpdateView) class.

[btbonval]

Manually test that:

• [x] anonymous user cannot edit or delete a note
• [x] non-admin user cannot edit or delete a non-owned note
• [x] non-admin user can edit and delete an owned note
• [x] admin user can edit and delete a non-owned note
• [x] admin user can edit and delete an owned note

[btbonval]

did that, then found out in two different places that the user also needs 20 points to edit his or her own notes and tag them.

Updated the single, monolithic edit check for that. I previously ignored tagging as I thought that was independent of note editing. I guess not. Alright, now a bunch more tests for all those conditions.

Cross product of (admin?)x(20 points?)x(own note?)x(note or tag?) = 16 cases. This is where I really wish the test suite actually worked for me. I'm not testing all that manually, and writing tests won't help because I can't run them.

So let's pick the important ones and use some of the previous tests to eliminate tests here (even though the code changed a little).

• [x] admin=yes, points=don't care points, own note=no, tags ::- edit
• [x] admin=no, points=<20, own note=yes, tags ::- no edit
• [x] admin=no, points=<20, own note=yes, note ::- no edit
• [x] admin=no, points=>20, own note=yes, tags ::- edit
• [x] admin=no, points=>20, own note=yes, note ::- edit
• [x] admin=no, points=>20, own note=no, tags ::- no edit
• [x] admin=no, points=>20, own note=no, note ::- no edit

[btbonval]

Just realized note editing relies on the note being editable, while tagging does not.

Pulling out some of the edit code into a tag permission, making edit permission use tag permission, and need to retry the above 5 tests.

Minimal additional tests for non-editable notes.

• [x] admin=yes, points=don't care, own note=don't care, tags ::- edit
• [x] admin=yes, points=don't care, own note=don't care, note ::- no edit
• [x] admin=no, points=<20, own note=yes, tags ::- no edit
• [x] admin=no, points=<20, own note=no, tags ::- no edit
• [x] admin=no, points=>20, own note=yes, tags ::- edit
• [x] admin=no, points=>20, own note=no, tags ::- no edit

PS, adding (editable?) to the cross product above resolves to 32 cases for proper testing.

[btbonval]

Looks like editable notes are being handled by the allows_edits_by function as appropriate, except it also uses that for tags from my debug, even if the tags changed but the body did not. That's alright because the result is the same.

Unfortunately, trying to change tags on a non-editable note bypasses by logic entirely. I replaced request.user.is_authenticated() and request.user.get_profile().can_edit_items() with note.allows_tags_by(request.user) here: https://github.com/FinalsClub/karmaworld/blob/note-editing-merge-more-ocw/karmaworld/apps/notes/views.py#L331

It does not appear to be firing, or it is being shortcut by request.method == "POST" and request.is_ajax(). Uncertain.

[btbonval]

Ah, edit note tags is a special form which operates differently than the edit notes (which includes tags). I had to remove the template logic.

I'm a little bothered by the redundance.

[btbonval]

tag handling problem fixed in #423

[btbonval]

tag window doesn't close when AJAX returns bad. In theory, the tag window wouldn't even appear, but it won't hurt to program it to close and display the error message, rather than do ... nothing.

[btbonval]

ugh that was a mess. All tests pass. Buttons are present even if unusable for testing purposes.

Need to:

• [x] confirm javascript error messages can close the tag window ... collect static :(
• [ ] hide or disable buttons which will have no effect (prefer disabling)

[btbonval]

Collected static. It sent the plain text up, but we're using compress for this. Newest compress version is from 2 weeks ago. It didn't make a fresh compress for some reason. Manually compressing and collecting static again.

[btbonval]

crap. I wrote checking methods which take an argument. can't pass arguments into functions using django templates. http://stackoverflow.com/questions/1529550/a-way-to-use-method-parameters-in-django-templates

The general workaround is to make a filter.

I suppose I could filter the user through the note's allows_this_by() method. Kind of makes sense. Returns True or False, drop in an {% if %}.

Wow that was quick and easy and works.

[btbonval]

This was resolved with the commit linked above.