Require an edit cookie to be able to view a comment to be edited.

[fluffy-critter]

The /id GET endpoint is only used to retrieve the raw text of a comment to be edited. Being able to arbitrarily retrieve any comment through this endpoint is a potential privacy issue, since it allows a malicious actor to individually retrieve all comments from a website without knowledge of an existing thread URI.

This change requires that comment text retrieval has a valid edit cookie.

Fixes #679

[ix5]

LGTM, but please rebase and remove the now unnecessary formatting commit.

---

I've verified this PR via the following methods:

Web interface

Creating a comment manually and editing it -> works

curl denied

Creating comment, note id=1, try curl 'http://localhost/id/1 -> 403 (as expected)

curl with cookie

Create comment, note id=1, cookie 1=WzIsImUzYjgyMDQwNTY1YmI0YmU2YjExZjc3OGEyZTNkZjMyN2VkMjBhM2IiXQ.YB_Puw.PjiKthPk4XotF-22aV11EoveROw, try:

curl 'http://localhost:8080/id/1' --cookie '1=WzIsIjUwOWU2YzJkNTFlZDRkNjkwNjA5ODcxNzQzODljNzlhNzBlYjQ1NWMiXQ.YB_PKw.GPKGtzNFiHKIwxc3i3U1olTj2jY'

->

{"id": 1, "parent": null, "created": 1612697387.4656265, "modified": 1612697531.182028, "mode": 1, "text": "<p>Now</p>", "author": "Nameee", "website": "http://web.site", "likes": 0, "dislikes": 0, "notification": 0}

As expected, works

nosetests

nosetests --with-doctest --with-coverage --cover-package=isso --cover-html isso/
................................................................................
Name                        Stmts   Miss  Cover
-----------------------------------------------
isso/__init__.py              164     57    65%
isso/config.py                 77     11    86%
isso/core.py                   68     20    71%
isso/db/__init__.py            66      6    91%
isso/db/comments.py           135     20    85%
isso/db/preferences.py         17      0   100%
isso/db/spam.py                36      0   100%
isso/db/threads.py             15      1    93%
isso/dispatch.py               39     23    41%
isso/ext/__init__.py           10      0   100%
isso/ext/notifications.py     150    100    33%
isso/migrate.py               201     27    87%
isso/run.py                     5      0   100%
isso/utils/__init__.py         75      8    89%
isso/utils/hash.py             67      3    96%
isso/utils/html.py             50      1    98%
isso/utils/http.py             39     25    36%
isso/utils/parse.py            43      2    95%
isso/views/__init__.py         36      8    78%
isso/views/comments.py        464     70    85%
isso/wsgi.py                  103     23    78%
-----------------------------------------------
TOTAL                        1860    405    78%
----------------------------------------------------------------------
Ran 80 tests in 5.242s

OK

[ix5]

Begone!

[fluffy-critter]

There's quite a few functions with that inconsistent spacing. Also technically the docstrings should be inside the functions, not outside of them. Fixing that feels a bit out-of-scope for this change. Maybe in the future there should be a docstring normalization pass.

[fluffy-critter]

(That said, I'll remove that accidentally-introduced newline. No idea how that got in there!)

[jelmer]

Yeah - I think there's value in fixing some of the formatting, but mixing those changes in with other changes just makes everything harder to review.

[fluffy-critter]

A thing I’ve found to be helpful is to make autopep8 part of the build process so that there’s never an opportunity for things to drift in the first place.

[jelmer]

Sure, but that should also be a separate PR :)

[fluffy-critter]

PR feedback applied.

[fluffy-critter]

It'd still be swell if this could get merged in.

[aquatix]

Is there something keeping these improvements from being merged?

[fluffy-critter]

The only objection I saw was from a stray formatting commit, which I've now rebased away.

[fluffy-critter]

That's a lot of CI failures though. None of the errors seem to have anything to do with my changes but I had rebased off master so maybe something weird happened.

[ix5]

Hmm, those CI failures do look like a pypi availability issue...

@fluffy-critter maybe rebase again to trigger another CI run?
@jelmer would you mind merging this afterwards?

[fluffy-critter]

Looks like the same errors are occurring.

[ix5]

The looks like the latest Flask package in pypi (version 2.0.2 as of 2021-12-22) only supports python 3.6 and newer.
The latest release to support python 3.5 is https://pypi.org/project/Flask/1.1.4/

Since the latest PR to get merged also has the CI runs failing, I'd advocate merging this PR since it introduces no CI regressions.

[ix5]

Also, @fluffy-critter you inadvertently (?) slipped some local commits (5 in total) in while rebasing, please get rid of them.

[fluffy-critter]

Ah, yeah, totally accidental. Will fix.

[jelmer]

This seems unrelated, or do you need newer versions of these for this change?

[fluffy-critter]

I’m not sure how that change snuck in, I thought I’d reverted that file to the one on master.

[jelmer]

Yeah - I think there's value in fixing some of the formatting, but mixing those changes in with other changes just makes everything harder to review.

[fluffy-critter]

Oh, yeah, I absolutely agree.

…

--- fluffy (she/her) https://beesbuzz.biz/

On Dec 22, 2021, at 11:25 AM, Jelmer Vernooij ***@***.***> wrote: @jelmer commented on this pull request. In isso/views/comments.py <#683 (comment)>: > @@ -954,6 +960,7 @@ def count(self, environ, request, uri): @apiSuccessExample Counts of 5 threads: [2, 18, 4, 0, 3] """ + Sure, but that should also be a separate PR :) — Reply to this email directly, view it on GitHub <#683 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACUYGVMLNJL4RYHZ7UGCKDUSIQ2JANCNFSM4WEWI2MQ>. You are receiving this because you were mentioned.

[fluffy-critter]

PR has been cleaned up and only has PR-specific changes in it now. Sorry about the temporary mess!