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

Protect anonymous comments from being changed by other users #1235

Merged
merged 2 commits into from
May 10, 2022

Conversation

paleale
Copy link

@paleale paleale commented Jan 19, 2022

Direct auth from go-pkgz/auth used in Remark42 for anonymous authentication. The problem is that as soon as a user logs in with the same name as another anonymous user, they get precisely the same user ID and can delete or alter messages by that other user.
To prevent that, we need to provide custom UserIDFunc, which I propose should return hashed IP of the user so that there will be no way another anonymous user and I get the same ID at the same time, but it would be possible to re-login right away with the same ID if I want to adjust my anonymous comment.

Issue description has been originally provided by @paskal

Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the pr!

I think it makes sense to extend user id returned by AddDirectProviderWithUserIDFunc with X-Forwarded-For and X-Real-IP. Pls note - we likely should also handle chains of reported ips from those headers as each proxy level may append its ip.

Probably smth similar to this

@paskal paskal added this to the v1.9 milestone Jan 20, 2022
@paskal paskal changed the title Use custom userID Protect anonymous comment from being changed by other users Jan 23, 2022
@paskal paskal changed the title Protect anonymous comment from being changed by other users Protect anonymous comments from being changed by other users Jan 23, 2022
@umputun
Copy link
Owner

umputun commented Feb 18, 2022

@paleale pls let me know if you are planning to make the proposed change

@paleale
Copy link
Author

paleale commented Mar 10, 2022

@umputun I am, back again, still uncertain about all the possible force majors, but I'd like to finish that at nearest holidays.

@akellbl4 akellbl4 modified the milestones: v1.9, 1.9.1 Apr 9, 2022
@umputun
Copy link
Owner

umputun commented May 10, 2022

My comment was probably inaccurate, or i have added it before we had RealIP middleware added. With this middleware in place your user + r.RemoteAddr should be fine. Pls, make sure the conflict resolved and I'll merge

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #1235 (2f01561) into master (168a6d3) will not change coverage.
The diff coverage is n/a.

❗ Current head 2f01561 differs from pull request most recent head e47f481. Consider uploading reports for the commit e47f481 to get more accurate results

@@           Coverage Diff           @@
##           master    #1235   +/-   ##
=======================================
  Coverage   50.19%   50.19%           
=======================================
  Files         139      139           
  Lines        3008     3008           
  Branches      642      642           
=======================================
  Hits         1510     1510           
  Misses       1490     1490           
  Partials        8        8           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 168a6d3...e47f481. Read the comment docs.

Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thx

@umputun umputun merged commit b82ed82 into umputun:master May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants