As per discussion with chx in IRC today, the delivery callback section of hook_menu() documentation, and the drupal_deliver_page() function documentation both need to have a warning about access checks.

The warning needs to get across the point that the delivery callback in hook_menu() or the delivery function in drupal_deliver_page() will be called even if the hook_menu() router item access callback returns FALSE.

Wording I suggested in IRC - for drupal_deliver_page:

Note that this function does not perform access checks. The delivery callback function will be called even if the router item would not grant access to the current user. This is intentional (needed for JSON and other purposes), but it has security implications. Do not call this function unless you understand the security implications.

for hook_menu:

Note that your delivery callback function should verify access before delivering content. It will be called even if the access callbacks in the menu router item return FALSE. See drupal_deliver_html_page() for an example.

ksenzee suggested putting them both in BLINK tags. Hah.

CommentFileSizeAuthor
#2 842962.patch4.2 KBjhodgdon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Well, delivery callback does not need to verify access much rather it should only perform operations relevant even if access would be denied. Like adding headers.

Also, the drupal_deliver_page documentation needs to mention something like "the page_delivery_callback defaults to the current router item delivery callback even if the router item is access denied".

jhodgdon’s picture

Status: Active » Needs review
FileSize
4.2 KB

Here's a patch for the doc to these two functions. Also a small amount of additional cleanup in drupal_deliver_page() doc.

jhodgdon’s picture

#2: 842962.patch queued for re-testing.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Sounds good.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Status: Fixed » Closed (fixed)

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