[18.0][IMP] privacy_consent: require explicit confirmation (POST) before recording the answer - #95
Open
fidpa wants to merge 1 commit into
Open
Conversation
…cording the answer The public consent route recorded the subject's answer on a plain GET. Mail gateways and antivirus link scanners prefetch every URL found in an e-mail, and every consent e-mail contains both the accept and the reject link, so the recorded answer ended up being whichever link a scanner fetched last, not a valid record of the subject's consent. A GET on the consent URL now renders a confirmation page; the answer is recorded only when the subject submits it (POST to the same URL). URLs are unchanged, so links in already-sent e-mails keep working, and subjects can still change their answer later.
Author
|
Ping. This has been open since 17 July, CI is green and there is no review yet. @michelerusti you did the 18.0 migration of this module, and @ivantodorovich you got #94 moving a while back. Could either of you take a look? Thanks in advance. What it does: the consent link in the notification mail records the answer on GET, so a link prefetch, a mail scanner or a proxy can answer on behalf of the data subject. The PR adds an explicit confirmation step (POST) and keeps GET write-free. Tests and the runboat build pass. One thing worth coordinating: #97 migrates privacy_consent to 19.0 and touches the same controller. If this one goes in first, I can forward-port it. If not, it has to be redone on top of the migration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The public consent route records the subject's answer on a plain GET
request (
controllers/main.py,consent()callsaction_answer()unconditionally). Every consent e-mail contains both the tokenized
Accept and Reject links (enforced by the
mail.templateconstraint).Corporate mail gateways and antivirus link scanners prefetch every URL
found in an e-mail with GET requests. With the current behavior this
means:
never gave (
state="answered",accepted=True).last. It is effectively random and worthless as proof of consent under
GDPR Art. 7(1).
out blacklist sync) fires based on that false answer.
This is a known weakness of "click-the-link-is-the-answer" flows; RFC
8058 exists for the analogous mailing-list unsubscribe problem, where
one-click unsubscribe uses POST for exactly this reason.
Solution
Record the answer only on an explicit POST:
/privacy/consent/<choice>/<id>/<token>now renders aconfirmation page (new QWeb template
privacy_consent.confirm) showingthe partner, the activity and a single confirmation button. It writes
nothing.
action_answer()asbefore and renders the existing thank-you page.
The URL scheme is unchanged on purpose. All consent links in already-sent
e-mails keep working (subjects get one extra confirmation click), the
default mail template and the
mail.templateplaceholder constraint areuntouched, and the "change your mind" links on the thank-you page keep
working the same way.
Notes for reviewers
(e.g. custom automation hitting the links) must now POST to the same
URL. That GET write is exactly the forgery vector this PR closes, so no
compatibility switch is provided. A setting that re-enables recording
consent from scanner prefetches would be hard to justify. Human subjects
are unaffected beyond the extra click.
csrf=Falsedeliberately, with the rationalein the controller docstring: the HMAC token in the URL is the credential
that authorizes the answer. A CSRF attacker would need to know the
token, and with the token they can POST directly anyway. Requiring the
session-bound CSRF token instead would hard-fail subjects whose browsers
block cookies (plausible for privacy-minded recipients of a consent
request). This is strictly stronger than the status quo, where a mere
<img src=...>triggered the write. Unlike single-shot token links (e.g.core's
/mail/unfollow), this is a two-step GET→POST flow in the samesession, so a per-session CSRF token could in principle be embedded on
the confirmation page, but that would reintroduce exactly the cookie
dependency
csrf=Falseavoids. If maintainers prefer the standard CSRFtoken embedded in the form despite the cookie dependency, I am happy to
change it.
18.0.1.0.0→18.0.1.1.0. Classified[IMP]with aminor bump rather than
[FIX]/patch because it adds new user-facingbehavior (a confirmation page), not only a bug patch. No data model
changes, no migration scripts needed.
("I accept/reject this processing of my data"); only the heading and one
explanatory sentence are new.
.potregeneration is left to the OCAexport/Weblate infrastructure as usual.
oca-gen-addon-readmepre-commit hook because the readme fragmentschanged. That hook also reformats them (list indentation, badge/banner);
none of it is hand-edited.
(
privacy_document_consent) covering a related use case (consent todocument versions), which I intend to propose to OCA later. That module
differs from
privacy_consenton several axes, and this GET-vs-POSTbehavior is one of them. For transparency: this PR removes one
differentiator between the two ahead of my own proposal. I am submitting
it regardless, because the scanner weakness is a real defect in
privacy_consentthat deserves fixing on its own merits, independentlyof my module.
Tests
test_consent_get_does_not_answer: the scanner scenario. Repeated GETson both links leave the consent untouched (
draft, not accepted, nometadata).
test_consent_post_answers: a POST records the answer exactly once, andchanging one's mind via the other link still works (undo flow
preserved).
test_generate_manually: updated so the full flow now confirms viaPOST. All existing assertions (state transitions, blacklist sync,
chatter subtypes, tokenized links not leaking into the thread) are kept.
test_consent_controller_security_noaccess: extended so invalidid/token combinations return 404 for POST as well as GET.
All module tests pass against Odoo 18.0 (test evidence available in the PR
conversation on request).