HTTP GET requests should be "safe". I can't recall from where this rule was (w3c?), but I learned about it because of google accelerator. From http://en.wikipedia.org/wiki/Google_Web_Accelerator - Technical Issues:

While the Web Accelerator works well on public websites, it can be damaging to some administrative web applications. For example, a web application may generate a page with "delete" or "cancel" hyperlinks...

In short, drupal should not allow changes to node revisions (or anything else) via GET requests. If one visits 'node/N/revisions' given time a web accelerator will most likely revert and/or delete all revisions (prefetching all simple links). There should be at least a confirmation which requires POST response.

CommentFileSizeAuthor
#10 revisions-confirm_1.patch9.77 KBChrisKennedy
#9 revisions-confirm_1.patch.txt9.81 KBdrewish
#7 revisions-confirm_0.patch9.79 KBkkaefer
#6 revisions-confirm.patch9.2 KBkkaefer

Comments

dman’s picture

dman commented

This is true, and it's a strict convention that certainly should be followed.
OMG, Google Ate My Website (AKA "The Spider of Doom")
Pity it's so fiddly to convert from links to action forms.

(damn, it reminds me that the 1-click update links I coded yesteday are bad mojo)
.dan.

killes@www.drop.org’s picture

killes@www.drop.org commented
Version: 4.7.2 » x.y.z
Category: bug » feature
Priority: Critical » Normal

if you let google or anon users delete revisions you get what you deserve...

making this a feature request vs. head.

mr700’s picture

mr700 commented
Category: feature » bug
Priority: Normal » Critical

killes, I thknk you miss understood me. It is not the google bot that will delete or revert, it's the google's accelerator program (and not only). It is a client side program works with your browser 'prefetching' web links when you browse the net (or your drupal site logged in or not). The idea is that your browser (with the help of this program) will download all links from the current page (while idle) so when you click one of them you will get it instantly from the cache.

Some more info - http://blog.moertel.com/articles/2005/10/25/google-web-accelerator-vs-un...

Please read the technical issues discussed at http://en.wikipedia.org/wiki/Google_Web_Accelerator and correct me if I'm wrong. Maybe it's not critical, but it's a bug.

killes@www.drop.org’s picture

killes@www.drop.org commented
Priority: Critical » Normal

well, considering this, I'd accept a patch for 4.7, but I maintain it isn't critical. It is also not Drupal's fault.

mr700’s picture

mr700 commented

Two more links from w3c: HTTP/1.1 Method Definitions - http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.1.2 and
GET must not have side effects - http://www.w3.org/DesignIssues/Axioms#state.

kkaefer’s picture

kkaefer commented
Assigned: Unassigned » kkaefer
Status: Active » Needs review
StatusFileSize
new revisions-confirm.patch9.2 KB

I added a confirmation page to both ‘revert’ and ‘delete’. I also got rid of the node_revisions() function (which acted as a switch between all the actions of revisions) and replaced it with dynamically added menu items. The performance should also be a bit better (although it doesn’t matter that much for the revision administration interface) because nodes are no longer loaded twice or even three times.

kkaefer’s picture

kkaefer commented
StatusFileSize
new revisions-confirm_0.patch9.79 KB

Renamed node/N/revisions/M/... to node/N/revision/M/... which is grammatically more correct than the plural form. node/N/revisions is still in its old place.

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: x.y.z » 5.x-dev
Status: Needs review » Needs work

patching file modules/node/node.module
Hunk #1 FAILED at 1102.
Hunk #2 succeeded at 1637 (offset 83 lines).
Hunk #3 succeeded at 1666 (offset 83 lines).
Hunk #4 succeeded at 1683 (offset 83 lines).
Hunk #5 succeeded at 1711 (offset 83 lines).
Hunk #6 succeeded at 2343 (offset 87 lines).
1 out of 6 hunks FAILED -- saving rejects to file modules/node/node.module.rej

This should be fixed if it is still an issue.

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new revisions-confirm_1.patch.txt9.81 KB

here's a re-roll, with proper title capitalizations (which was what had broken the last patch)

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new revisions-confirm_1.patch9.77 KB

Works great. While I was at it I added two periods to doxygen comments.

mr700’s picture

mr700 commented

Yep, works great, thanks!

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.x-dev » 6.x-dev
ChrisKennedy’s picture

ChrisKennedy commented
Status: Reviewed & tested by the community » Needs work

Needs to be updated to the new menu interface.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

Still needs to be fixed in the 6.x-dev branch.

cburschka’s picture

cburschka
they
commented
Status: Needs work » Fixed

The revision-deleting and reverting options on the overview appear to be already guarded by a POST confirmation form. If there is another security hole that allows deleting via GET, please re-open.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.