Problem/Motivation
The contact form allows users to change the name and mail fields. This can be considered a security issue in that recipients of the message may be tricked into communicating with an impostor.
Proposed resolution
Remove the ability for logged in users to change name and email address on the contact form.
Remaining tasks
- Minor cleanups.
- Manually test the email sent for both the sitewide and personal contact forms for both anonymous and authenticated users.
User interface changes
Only anonymous users will be allowed to enter a name and email address on the contact form. Users with an account will be forced to use the settings from their user data. Text will alert recipients of the sent emails that the anonymous users are not verified.
Before patch
After patch
String addition
The string t('Unverified')
is added to contact form emails sent by anonymous users.
API changes
The structures of contact_site_form()
and contact_personal_formm()
change for authenticated users (replacing text fields with read-only values).
Original report by Dave Reid
Currently, both the site-wide and personal contact forms allow registered users to change the values of the 'name' and 'mail' fields. So I could submit a user's personal contact form with the name 'Dries' and 'dries-not-valid-mail@drupal.org' and it would look to the user like this e-mail actually came from Dries, which is a bad thing.
These forms should both remove the fields when a non-anonymous user is using them, and instead use theme_username() to display that the e-mail will be 'sent' from the current user.
Comment | File | Size | Author |
---|---|---|---|
#155 | 601776-contact-core-134.patch | 5.3 KB | stefan.r |
#135 | 601776-contact-core-134.patch | 5.3 KB | dcam |
#131 | before.PNG | 3.74 KB | dcam |
#131 | after.PNG | 4.33 KB | dcam |
#130 | 601776-contact-core-130.patch | 1.32 KB | David_Rothstein |
Comments
Comment #1
Dave ReidBasic patch to display theme_username() instead of textfields for name and e-mail for registered users on both site-wide and personal contact forms. My only concern is that maybe admin users ('administer contact forms' for the site-wide and 'administer users' for personal contact forms) should maybe be able to enter a different e-mail address, so say if I contacted a user on drupal.org about a security issue and wanted to set 'my' email address as security@drupal.org.
Comment #2
xmacinfoThis is not a good idea to remove entirely this feature. Yes, this is a feature.
For some web sites, we need to use generic user names. For example for a presentation, or a download area. So instead of requiring each user to create an account, we often create a generic username.
To help out these user, we also enable the contact form to send us a message, where they can go and type their name and their email address by replacing the values set for the generic user name.
So please make this a setting or a permission.
However, the more and more I am using the contact module, the more and more I think that for our clients we will enable the webform module.
So if we are using the 80% rule I see often in the issue queue, I would agree with you and ask the 20% use cases administrators to install webform.
So in addition to your concern about admin users, I have concerns for generic user names as well. :-)
What do you think?
Comment #4
antelrope CreditAttribution: antelrope commentedAllowing a user to enter a different name and email address is not necessarily a bad thing. It is a feature that I and others, it seems, want on their site. Some may not want this functionality. That is fine. This should be a configuration option.
I do advocate always including what account the email was sent from as part of the email.
What does everyone else think?
Comment #5
Dave ReidGreat thing is you can always un-lock down these fields via a quick form_alter, but I think it's a good idea to lock them down.
Comment #6
xmacinfoWell make it an admin contact setting then, defaulted to locked. :-)
Comment #7
antelrope CreditAttribution: antelrope commentedEven better would be a permissions setting, that way it could be restricted or allowed based on roles. The only pitfall I see is that it doesn't make sense to dissallow an anonymous user from changing their name and email. Is there a way to make a permission box sticky and always checked for a particular role? If not, the only workaround I can imagine for this is to, in the code, check first for permission and if that fails, check to see if the user is anonymous, if so, grant permission anyhow.
Comment #8
xmacinfoLet's make this 8.x-dev.
This way we will have more time on how to properly handle generic users versus specific ones.
The current solutions does not offer any real fall back. And for end users, it's not always possible to modify the theme.
Comment #9
gregglesI think the features available by default in core should not create a situation that allows for false e-mails to be sent,which is what this bug does.
If someone wants to configure webform to let webform do that then they have to actively make that decision, but core shouldn't, by default, allow impersonation. As Dave says, if someone really wants to use the contact module and not enforce this then they could do a form_alter.
I don't think it should be exposed as an option. If we expose everything the system becomes too complex. I don't think the use of this feature is very common (I've never seen it used).
Comment #10
Dave ReidYeah this just makes no sense to allow users to change information on a contact form that needs to be validated when they change their account info.
Comment #11
sunSorry, but: http://api.drupal.org/api/drupal/modules--contact--contact.pages.inc/fun...
May be confusing or annoying, but not a bug.
Comment #12
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #13
sunAlrighty, @Dave Reid made sure to "highlight" the current situation in a very subtle way ;)
In turn, this can be considered as a publicly disclosed security vulnerability. Thus, let's make sure to fix this soon and backport to D7 and D6.
Comment #14
gregglesJust so the reputations of the innocent are not damaged, I made that email ;)
Comment #15
dinarcon CreditAttribution: dinarcon commentedthe attached patch is basically the same as #1 with the following modifications:
- removed $form['#token'] according to recommendation at http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....
- use of
format_username($user)
instead oftheme('username', array('account' => $user))
Although this method prevents authenticated users from entering a name and e-mail, it does not prevent anonymous users to do so. Maybe we could add a validation that prevents anonymous users to enter an e-mail address that belongs to a registered user. Could adding a similar validation for the name field be too much? Probably anonymous user handling might go into its own issue.
edit: the file affected by this patch haven't changed between D7 and D8 so the same patch could be applied on both branches
Comment #16
webchickTests, plz!
Comment #18
xmacinfo@dinarcon: That would mean that a user would need to log in to post a message whereas anonymous users would be able, at anytime, to send a message. I don't think we need to go that far.
What we need is a way to know if the message comes from a registered user or from an anonymous user.
Comment #19
pillarsdotnet CreditAttribution: pillarsdotnet commentedlike the (verified) or (unverified) attached to comments, right?
Comment #20
xmacinfo@pillarsdotnet: Indeed. The security issue here is impersonation. Adding (unverified) would solve this. Or a better wording.
Comment #21
pillarsdotnet CreditAttribution: pillarsdotnet commentedVery minimal patch to satisfy the security issue.
Comment #22
pillarsdotnet CreditAttribution: pillarsdotnet commentedSlightly better.
Comment #23
gregglesI'm not aware of a problem that calls for strip_tags. Can you expand on the need for that?
Comment #24
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe output of
format_username()
may include a link to the user profile page, or other tags. The "From" line of an email message will display as plain text, not html. The api page recommends callingcheck_plain()
, but I'd preferFrom: Bob Vincent (pillarsdotnet)
to
From: Bob Vincent (<a href="/drupal/user/36148">pillarsdotnet</a>)
in my emails. How about you?
Comment #25
pillarsdotnet CreditAttribution: pillarsdotnet commentedNow includes both username and email info, if they differ.
Comment #26
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdds
'(Verified)'
if the user info matches the submitted form values.So a contact email from me would look like one of the following, depending on my submitted form values:
From: pillarsdotnet (Verified)
From: Bob Vincent (pillarsdotnet)
From: pillarsdotnet (<bobvin@pillars.net>)
From: Bob Vincent (pillarsdotnet <bobvin@pillars.net>)
From: Bob Vincent (Anonymous)
EDIT: Removed irrelevant ramblings.
Comment #27
pillarsdotnet CreditAttribution: pillarsdotnet commentedTasks aren't critical, and I'm not even convinced that this one qualifies as "major", but splitting the difference.
Comment #28
xmacinfoI like very much the way you handle this issue.
Personally I would use the example above where we need to only translate on string: (verified). However, is it best to highlight the verified account or the (unverified) ones? In comments, we highlight only the unverified ones.
Comment #29
xmacinfoCross-posting.
Comment #30
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdded the "Not logged in" case to the tables in #26.
I don't think the comment form allows logged-in users to provide a name and email other than their own. But the contact form does, and some people don't want to remove that "feature".
Comment #31
Dave ReidNo, that's theme_username(), not format_username(), which always returns plain-text username only. Running strip_tags is unnecessary.
Comment #32
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay, but the API page calls it "unsanitized text". Is it safe to stuff into an email header?
Comment #33
sunI'd highly recommend to go back to the patch in #1.
The original approach is mostly identical to what happens in comment_form(). It doesn't make sense to expose a username and e-mail field for registered users. Sites with use-cases like @xmacinfo mentioned can happily use Webform or write a 4 lines hook_form_alter() to unlock the fields.
The only remark I have about #1 is that it should not use a conditional form structure for the 'name' field; i.e., the form element should always be contained, but #access => FALSE for authenticated users.
And lastly, we're still missing tests, of course.
Comment #34
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-titling then.
Comment #35
xmacinfo@Sun: Does patch in #1 let an anomymous user impersonate a user? When opening a message from a mailbox, how can we spot the difference?
Comment #36
pillarsdotnet CreditAttribution: pillarsdotnet commentedAttached patch:
#access
to!$user->uid
).' (Unverified)'
to the sender name for anonymous usage.If this is a satisfactory approach, I will write tests.
Comment #38
pillarsdotnet CreditAttribution: pillarsdotnet commentedAccount tests fail because they require what this patch forbids. Again, if someone agrees this is the correct approach, I will (re-)write tests.
EDIT: bump.
Comment #39
Dave ReidI'd rather use '#type' => 'item' for both name and e-mail for registered users, so we can at least remind people what the mail will look like it comes from, rather than just not displaying that information.
Comment #40
pillarsdotnet CreditAttribution: pillarsdotnet commented@Dave Reid:
Are you voting for the patch in #32 or for something different?EDIT: Now I see what you mean. Will roll another patch.
Comment #41
pillarsdotnet CreditAttribution: pillarsdotnet commentedAttached patch implements the suggestion of Dave Reid In #39.
Again, I expect tests will fail, because they require what we now want to forbid. If we can agree that this is the correct approach, I will re-write tests to match.
Comment #43
xmacinfoI like the way you are approaching this issue. I thank you for the addition of the (Unverified) string.
From my point of view you can start building the tests.
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedGood to see someone on this. So, say I wanted to disable the ability for registered users to change the e-mail or name on the contact form in D7, which one of these patches will allow me to do this? I realise that it's nice to have the option, especially for anonymous users, to change these things, but I want a 'disable custom user name/e-mail' check box in the contact form admin panel.
Comment #45
xmacinfo@Borden Rhodes: The last patch should do the trick. But you’d need to reroll it for Drupal 7.
Comment #46
Jon PughTests!!
only tests for logged in users so far.
Comment #47
Jon PughComment #48
Jon PughHeres the fix and the tests. I have to leave, so sorry if I forgot additional lines! Its not an exact re-roll, but it works
Comment #50
Jon Pughnew patches...
Comment #52
Jon PughThe fix doesn't quite work yet, still have to pull the values on submit.
Here's a patch just for the test, hopefully it fails properly :D
Comment #53
Jon PughComment #55
Jon PughTest failed successfully!
Attached is what is hopefully a final patch. Emails are sending, personal form is also now patched.
Comment #57
BerdirFixed the comment coding style and fixed the failing tests.
Comment #58
xjmThanks @careernerd and @Berdir. The test looks good.
I found a few more minor stylistic issues:
I'd suggest:
Log the current user out in order to test the name and email fields.
The docblock summary should be a single line of less than 80 characters. Also, I'd suggest changing the text here (and in the
getInfo()
and test method docblock) because the test doesn't actually have anything to do with anonymous users.I'd suggest:
ContactAuthenticatedTestCase
Tests the contact form for authenticated users.
getInfo()
accordingly.This comment needs a period. It could also be a little clearer. Maybe:
Tests that name and email fields are not present for authenticated users.
The assertion messages here need not be translated. Reference: http://drupal.org/simpletest-tutorial-drupal7#t
These are straightforward cleanups, so tagging novice for the task of making those changes. The correct failures of the added test are already exposed in #53.
Comment #59
xjmWait, what happened to the "Unverified" string? It seems to have disappeared in the latest patches but I don't see a justification for that change.
Also, couple more minor points for cleanup:
This comment needs a space after
//
and a period at the end. Reference: http://drupal.org/node/1354#inline"Read-only" should be hyphenated.
Once the patch is fixed for the points above, let's also get some screenshots of the way the form looks for anonymous vs. authenticated users for reviewers, and an updated summary while we're at it. Note: please make screenshots in a small browser window and crop only to the relevant portion of the screen.
Comment #60
lliss CreditAttribution: lliss commentedAdded an issue summary.
Comment #61
lucascaro CreditAttribution: lucascaro commentedHere are some screenshots for anonymous and authenticated users both before and after applying the patch.
Anonymous user, before the patch:
Authenticated user before the patch:
Anonymous user, after the patch:
Authenticated user after the patch:
Comment #62
lucascaro CreditAttribution: lucascaro commentedhere are some screenshots for the user contact form (the ones in #61 are for the site-wide contact form). These are in the same order as before.
Comment #63
Zgear CreditAttribution: Zgear commentedabout to reroll patch with changes, gimme a sec.
Comment #64
Zgear CreditAttribution: Zgear commentedHere is the patch with the changes suggested and an interdiff :)
Comment #65
Zgear CreditAttribution: Zgear commentedComment #67
Zgear CreditAttribution: Zgear commented#64: contact_forms_security_601776-63.patch queued for re-testing.
Comment #69
Zgear CreditAttribution: Zgear commented:( ok let me take a look at it
Comment #70
Zgear CreditAttribution: Zgear commentedsorry about that guys, this should do the job
Comment #71
Zgear CreditAttribution: Zgear commentedah my bad, here it is with the correct name
Comment #72
Zgear CreditAttribution: Zgear commentedComment #73
Kevin Morse CreditAttribution: Kevin Morse commentedVerified the behaviour of the forms illustrated above is working in all four test cases.
I do not have the ability to send mail from my local test server so I cannot comment on the appearance of the emails themselves.
Comment #74
xjmFew more minor cleanups:
This comment looks like it's wrapping too early. Each line should go as close to 80 chars as possible without going over. Also, let's remove the word "we" in accordance with our text guidelines (http://drupal.org/node/604342). I'd rephrase the comment as:
Do not allow authenticated users to alter the name or email values to prevent the impersonation of other users.
Unrelated change; let's remove this to reduce patch conflicts.
"Read-only" still needs a hyphen. :)
Additionally, when uploading the final patch, let's also upload a test-only version to expose the expected failures without the bugfix.
Once the patch is updated, it would be good if someone could test the sent emails manually, and double-check that the "unverified" string is there.
Finally, I'm updating the summary with the screenshots from #61. Thanks @lliss, @lucascaro, and @Zgear!
Comment #74.0
xjmAdded issue summary according to https://drupal.org/node/1155816
Comment #75
kbasarab CreditAttribution: kbasarab commentedUploading a revised patch taking xjm's suggestions into patch.
Trying to create the failure test though isn't working so well because DrupalWebTestCase is no longer in the recent head of core. I'm gonna reroll with changing the test case but wanted to get this one up with just the style changes.
Comment #77
kbasarab CreditAttribution: kbasarab commentedAlrighty lets try these. One that passes and one that will fail out testing just the test case.
Also an Interdiff between this patch and #71
Comment #78
kbasarab CreditAttribution: kbasarab commentedComment #80
xjmThat looks good to me. Thanks @kbasarab.
Now let's just have someone test sending an email as an anonymous user. If that works correctly I think this is RTBC.
Comment #81
damiankloip CreditAttribution: damiankloip commentedI will look at this and re-roll; contact.test doesn't exist anymore. Tests are now in ContactPersonalTest.php and ContactSitewideTest.php.
Comment #82
kwattro CreditAttribution: kwattro commentedHi,
I've re-rerolled the patch, applied the #77 with --reject option and made manually the changes to tests.
I've also created a new test class ContactAuthenticatedUser.php, actually tests are passing on my local 8.x .
Manually tested :
Comment #83
damiankloip CreditAttribution: damiankloip commentedI'll unassign myself then. Saves me some time :)
Comment #84
kwattro CreditAttribution: kwattro commented#82: contact_forms_security_601776-82.patch queued for re-testing.
Comment #85
kwattro CreditAttribution: kwattro commentedSent for Retest after the recent merge of the wscci/kernel initative
Comment #86
kwattro CreditAttribution: kwattro commentedI've rerolled the patch against the latest update of 8.x
Comment #87
xmacinfoWhite space after @file.
Needs a newline at end of file.
Comment #88
kwattro CreditAttribution: kwattro commentedFixed and rebased on the latest 8.x
Comment #89
kwattro CreditAttribution: kwattro commentedComment #91
kwattro CreditAttribution: kwattro commentedMissed something in the rebase :-(
Retry
Comment #92
SuperHeron CreditAttribution: SuperHeron commentedTested on a fresh d8. Looks OK. It could become RTBC.
Comment #93
jfhovinne CreditAttribution: jfhovinne commentedTested site-wide and user contact forms on a fresh D8, as anonymous user and authenticated user.
Ability for logged in users to change name and email address on the contact form is removed.
The string t('Unverified') is added to the user contact form emails sent by anonymous users, but NOT to the site-wide contact form.
Comment #94
tim.plunkette-mail, not mail
e-mail, not mail
There's an extra blank line here, and the Definition line is missing a trailing full stop.
The name shouldn't have a full stop, but the description should
Eventually all calls to setUp will expect an array, might as well switch this while rerolling.
Might as well combine this line, that will also avoid calling it $user.
$this->drupalLogin($this->drupalCreateUser(array('access site-wide contact form'));
e-mail, not email
Comment #95
kwattro CreditAttribution: kwattro commentedThanks @jfhovinne & @tim.plunkett for your review, will update it during the week.
Comment #96
disasm CreditAttribution: disasm commentedAttached is the changes requested with an interdiff of the last patch.
Comment #97
jfhovinne CreditAttribution: jfhovinne commentedTested latest patch on d8.
User interface is OK (4 cases: anon/auth - user/site-wide contact forms).
The 'Unverified' string (anonymous case) is added to the message sent while using the user contact form, but not to the message sent using the site-wide form.
I don't think this is the expected behaviour, so I attach the modified patch - now the string 'Unverified' is also added while using the site-wide contact form.
Comment #98
tim.plunkettJust a reroll and to see if the tests actually fail on their own.
Comment #99
dags CreditAttribution: dags commentedTested and can confirm that it works as intended, including jfhovinne's addition of Unverified to the site-wide contact form.
Comment #100
Dan Z CreditAttribution: Dan Z commentedGood stuff, and I hope to see it back-ported to D7 soon. Two important considerations:
First, it is still easily possible for authenticated users to spoof e-mail addresses via this form, even with this improvement. The user merely has to change his own e-mail address in his profile and then use the form. A bot could easily change it repeatedly, and use the "Send yourself a copy" checkbox to distribute spam.
A solution is available at http://drupal.org/project/email_required. It requires that you verify your e-mail address (respond to a message sent there) before using specified pages, and the contact page can specified. It appears to take effect on new (unverified) accounts, or after the e-mail address is changed.
Second, if hook_form_alter() or other means is used to modify the "Your e-mail address" value, for whatever reason, the "Send yourself a copy" feature should be disabled in most cases, as it could be used for spam. Instead of merely checking if it is an anonymous user, there should be a check to see if the e-mail address used for the contact function matches the e-mail address in the user profile.
Comment #101
webchickAt a glance this looks good to me. Committed and pushed to 8.x, thanks!
Marking to 7.x for backport.
Comment #102
tim.plunkettUnfortunate commit message is unfortunate.
http://drupalcode.org/project/drupal.git/commit/20d4ceb
Comment #103
Dan Z CreditAttribution: Dan Z commentedI "back-ported" this fix to D7 via a new module. This makes it available immediately, without waiting for the next core release. See http://drupal.org/sandbox/Z-TwistBooks/1704772 for the module.
For those not familiar with the git pages, the module can be downloaded by clicking on an appropriate "snapshot" link at http://drupalcode.org/sandbox/Z-TwistBooks/1704772.git.
Given that this is scheduled for backporting into D7 core anyway, is it worth making a full-blown project out of this?
Comment #104
gregglesI don't think so.
Comment #105
ZenDoodles CreditAttribution: ZenDoodles commentedBackported to D7.
Comment #106
ZenDoodles CreditAttribution: ZenDoodles commentedActually attach 'em this time.
Comment #108
ZenDoodles CreditAttribution: ZenDoodles commentedSorry for the spam. I missed a file earlier in the tests.
Comment #110
ACF CreditAttribution: ACF commentedFix for D7
Comment #111
ryan.gibson CreditAttribution: ryan.gibson commentedTested the patch from #110. Seems to work great. Can't alter the name and email after applied.
Comment #112
jaffaralia CreditAttribution: jaffaralia commentedReviewed and Tested the patch #110. It works perfectly. Authenticated user can't able to alter the name and email after this patch applied.
Comment #113
David_Rothstein CreditAttribution: David_Rothstein commentedIs this correct? Offhand, it sounds to me like it is (and for users with permission to change their own username the same thing goes for the name field too)...
If so, we need to discuss the security considerations here more. If there isn't much (or any) security benefit in the end, then we shouldn't backport this, because it makes changes to the form structure on two very public-facing forms, so per http://drupal.org/node/1527558 we shouldn't do it unless it fixes an important issue.
There might be some benefits of this patch either way (ones I can think of include: can't exactly spoof the name/e-mail of another registered user anymore, can't spoof the name anymore if you don't have "change own username" permission, can't spoof anything anymore if you stumble upon someone's already-logged-in account on a public computer) but those seem significantly less than the advertised benefits of this patch and also might be achievable with other, less intrusive changes... So yeah, let's please discuss this more.
****
A couple other things looking at the patch and while this is back on Drupal 8 anyway:
Why is this changing #type rather than setting #access FALSE? This is especially important for possible D7 backport, since it's a lot friendlier to any other code that's changing this via hook_form_alter().
Seems wasteful to call this function twice (especially since calling it invokes a hook), rather than storing the previous call from earlier in the same function in a variable and then reusing it here.
Minor code style (missing space before the brackets).
The newly added line of code doesn't seem to have anything to do with the code comment above it. (Because of that I almost thought there was a bug here where "Unverified" was being saved to the cookie, but there isn't.)
Also, this part of the patch looks like it might be a good to backport to Drupal 7 regardless of the rest, but why use "Unverified" here rather than "not verified"? (For consistency with the rest of Drupal, and, for Drupal 7 backport, for the sake of not adding a new string for no reason...)
Comment #114
Dan Z CreditAttribution: Dan Z commentedThere is is absolutely a good security benefit here and this fix should be backported.
There are two holes that let the user spoof e-mail. Hole #1 is changing the e-mail address via this form. Hole #2 is changing the e-mail address via the profile. Both holes need to be plugged.
This fix plugs up hole #1. That's a good thing. We still need to plug up hole #2, that's all. However, that's a different issue that should be addressed separately.
One way to plug hole #2 is to add some sort of verification when someone changes his e-mail address. For example, the module at http://drupal.org/project/email_required does this.
Comment #115
David_Rothstein CreditAttribution: David_Rothstein commentedFor Drupal 8, addressing them separately definitely makes sense. But for Drupal 7, we're talking about changing the behavior in a stable release for all sites that use the Contact module (and changing it in a way that not only changes the form structure but also removes a feature that according to several of the early comments in this issue, some people's sites are actually relying on)....
So the question is, if you need to install a contrib module to get a real security benefit from this anyway, then in Drupal 7, why not leave it up to that contrib module to change this behavior also, so that only sites which will need/want/benefit from the change actually get it?
Maybe we will decide that this patch actually does provide enough of a security benefit on its own that it's worth doing in Drupal 7. I just want to make sure we have that conversation.
Comment #116
Dan Z CreditAttribution: Dan Z commentedOh, it's worth backporting, and the sooner the better. I'm confused as to why e-mail spoofing for authenticated users was ever allowed in the first place through this form. I'm even more confused as to why it hasn't been fixed after nearly three years!
Very few administrators are going to want to allow spoofing. It's not even worth adding an "Allow authenticated users to use fake e-mail addresses" option to the admin/structure/contact form, because almost nobody would check it. Those few who do want spoofing can enable it via a hook_form_alter() similar to the module in #103.
Comment #117
Dan Z CreditAttribution: Dan Z commentedThe patch for 8.x has already been committed and pushed. Moving this back to 7.x. Please create a new issue for any issues with code committed to 8.x.
Regarding #113:
I don't see any difference with making the '#type' a 'value' and setting '#access' to false. Any module using hook_form_alter() can easily replace the whole field either way.
Your notes about the style and re-use issues seem valid. Please submit a new patch to fix them.
Comment #118
ACF CreditAttribution: ACF commentedReattached patch with small fixes.
Comment #119
jhoodI applied and tested patch #118 and everything seems to be working as it should.
Comment #120
David_Rothstein CreditAttribution: David_Rothstein commentedChanges still need to go into Drupal 8 first before being backported.
Some of the minor ones could definitely be split off to a separate issue, but not any of the ones that fundamentally affect the Drupal 7 backport (such as #type='value' vs. #access). And as long as we're moving it back for those, it's probably just a lot simpler to fix the minor issues at the same time.
Yes, but it complicates things considerably because normally a module will not do that. The standard way is to use #access, and if a module writes code to set a form element's access to TRUE or FALSE based on certain conditions, that code really should "just work" (rather than having to remember to do something different for this particular form element).
That's especially true for Drupal 7, where such code might already exist and we don't want to break it. The backport policy (see http://drupal.org/node/1527558 which links to http://groups.drupal.org/node/210973#comment-701613) mentions that changing #type is considered to be a pretty intrusive change.
Comment #121
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a followup patch for Drupal 8 addressing the full list of feedback from #113.
Comment #122
jaffaralia CreditAttribution: jaffaralia commentedHere i applied and tested the patch #121. Its working fine..
Comment #123
Dan Z CreditAttribution: Dan Z commentedThe matter of being able to change your e-mail address on your user profile page without verification is a known issue and being addressed at #85494: Use email verification when changing user email addresses. It could use more attention....
Comment #124
webchickThose all look like sensible clean-ups.
Committed and pushed to 8.x. Back to 7.x now.
And I'm really sorry, ACF, but I pressed the button too fast and accidentally didn't add your name to the commit message. :( Your next core patch, I will add you twice to make up for it. :D
Comment #125
ACF CreditAttribution: ACF commentedHave ported the patch to d7 using David_Rothstein's changes.
Comment #126
jhoodI applied patch #125 and it is working fine in D7.
Comment #127
dcam CreditAttribution: dcam commentedI tested #125 on D7 and it seems to work well. Before and after images are attached.
Comment #128
webchickLeaving this one for David, unless he gives the go-ahead for me to take it. Sounds like he had some concerns.
Comment #129
David_Rothstein CreditAttribution: David_Rothstein commentedYikes, I can't believe I missed this before, but both of these lines (the one from the original patch and the one my followup patch changed it to) are a security hole.
Comment #130
David_Rothstein CreditAttribution: David_Rothstein commentedThis should fix it.
Hopefully after that, we can actually put this issue back on Drupal 7 and be able to leave it there for a real discussion :)
Comment #131
dcam CreditAttribution: dcam commentedI tested #130 and it fixes the vulnerability. Before and after images are attached.
+1 for RTBC.
Comment #132
aspilicious CreditAttribution: aspilicious commentedLooks good :)
Comment #133
webchickOk, ONE last time, I hope. :D
Committed and pushed to 8.x. Thanks!
Back to you, David!
Comment #134
webchickOh, and reducing back down to just "major" now that the security vulnerability is dealt with.
Comment #135
dcam CreditAttribution: dcam commentedThis is the same patch from #125 for D7 with the security improvements from #130 added.
Comment #136
tim.plunkettCrosspost.
Comment #137
dcam CreditAttribution: dcam commentedOops, I didn't mean to change the status back to critical. The opened the page to post the patch before webchick downgraded it to major.
Comment #138
YesCT CreditAttribution: YesCT commented#135: 601776-contact-core-134.patch queued for re-testing.
Comment #139
YesCT CreditAttribution: YesCT commentedthis
looks like the issue summary is pretty up-to-date.
for the Minor cleanups... what detail can we write down?
so that a novice has more direction to create a patch (contributor task doc if it's helpful for someone: http://drupal.org/node/1424598)
and a help doc for the manually test in case someone wants to jump into this: http://drupal.org/node/1489010
... :) and in case the previous patch does not apply, reroll doc: http://drupal.org/patch/reroll
Comment #140
YesCT CreditAttribution: YesCT commented#135: 601776-contact-core-134.patch queued for re-testing.
Comment #141
YesCT CreditAttribution: YesCT commented#135: 601776-contact-core-134.patch queued for re-testing.
Comment #142
lovelykaushik86 CreditAttribution: lovelykaushik86 commented#1: 601776-contact-name-mail-fields-D7.patch queued for re-testing.
Comment #142.0
lovelykaushik86 CreditAttribution: lovelykaushik86 commentedUpdated issue summary.
Comment #143
gregglesI think at least this set of tags was removed accidentally. Not sure about the novice tag.
Comment #144
royal121 CreditAttribution: royal121 commentedPatch in comment #135 works fine.
Comment #145
mgifford135: 601776-contact-core-134.patch queued for re-testing.
Comment #146
blasthaus CreditAttribution: blasthaus commentedPatch in #135 works. but would it not be simpler to just make the textfields 'readonly' as this formatting will cause headaches for a lot of themes?
Comment #147
jukka792 CreditAttribution: jukka792 as a volunteer commentedIs this implemented in the core already?
I am still able to change the email and username (in the contact form) when sending message as authenticated user to another. Drupal 7.39.
Comment #148
xmacinfoThe best way to have this issue implemented in core is to review the code in comment #135, test it and report back.
Comment #153
dxvargas CreditAttribution: dxvargas commentedI tested and #135 works.
Nice code, tests passed!
@blasthaus I agree that a readonly field could be better for theming. But this issue has some security concerns associated, it is too old and we should just move on with what we have, it is a big improvement. Marking it as RTBC!
Comment #154
blasthaus CreditAttribution: blasthaus commentedWe abandoned using the contact module and just rolled our own after seeing some malicious attempts at sending POSTs to this path. It's amazing that this issue is 8 years old and still no love for D7. +1 for a commit before we break a record here :-)
Comment #155
stefan.r CreditAttribution: stefan.r commentedReuploading to trigger tests
Comment #158
xmacinfoWhat's happening with this issue and multiple commits? Last patch was failing.
Comment #159
gregglesThose are a new branch, 9.1.x. Whenever a new branch is opened up it happens like it did for 8.3.x and 8.4.x.