As noted by David Rothstein we should work on this patch in a public case. Latest patch attached.

Critical for RTM release.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

For reference, this is the followup for http://drupal.org/node/1441252.

I'm pretty busy the next few days, but will try to find some time to work on this soon.

hass’s picture

First bug report for the security changes #1442080: "permission restrictions" in reports -> run re-analyze only task once after upgrade

Additional to this I believe this changes will cause major troubles. If a link get's broken and it's used in let's say 1.000 or 10.000 nodes, all this nodes are loaded. Every installation will go out of memory... This need to be changed or we need to rollback the committed patch.

David_Rothstein’s picture

The code we added doesn't load all the nodes; it's supposed to stop after the first one in which the user has access to see the link. So the only time it should load lots of those nodes is if the user viewing the broken link report does not have access to the vast majority of them. I thought we decided that's a very unlikely scenario... Did you come across another way to trigger that?

I'll follow the internationalization issue now - nodes in different languages is definitely not something I tested :( So it sounds like there may be an issue that needs fixing there.

hass’s picture

I have no good idea how to solve the 10000 node load issue, but it will happen and this is extremly bad... :-(((. The i18n issue can be a false report... Maybe he really had no permission...

hass’s picture

tjmoyer’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

There doesn't seem to have been any progress or movement here in many months. I have been working on a module to tie in linkchecker with OG to provide a linkchecker report to groups managers, but found that linkchecker's queries for the report page don't respect node access.

I have created a patch that respects node access in the _linkchecker_report_page() function. This ensures that anyone who views a report only sees links to edit nodes they have permission to edit.

hass’s picture

Assigned: Unassigned »

I worked yesterday on the patch and fixed some bugs, but was not yet able to complete the node function. I hope to be able to post the patch soon.

hass’s picture

Status: Needs review » Needs work
hass’s picture

Status: Needs work » Needs review
FileSize
34.02 KB

I think I have finshed this task now.

@David_Rothstein: I know you are bussy with core, but is there a chance for a security review prior to commit?

Status: Needs review » Needs work

The last submitted patch, 1441574-Port-D6-access-bypass-bugfixes-to-D7.patch, failed testing.

hass’s picture

Status: Needs work » Needs review
FileSize
34.24 KB

Suxxx Eclipse Egit changes. New try with older Eclipse 3.7.2

David_Rothstein’s picture

Status: Needs review » Needs work

Thanks for reviving this issue. I reviewed the code (but did not test it out yet). Looked mostly good, but:

  1. +function _linkchecker_link_node_ids($link, $node_author_account = NULL) {
    ...
    +  // Get a list of nodes containing the link, using db_rewrite_sql() to allow
    +  // node access modules to exclude nodes that the current user does not have
    +  // access to view.
    

    db_rewrite_sql() doesn't exist in Drupal 7; the replacement for node access is $query->addTag('node_access'). So the two queries below this comment need to add that in order to actually allow node access to be enforced here.

  2. +function _linkchecker_link_comment_ids($link, $comment_author_account = NULL) {
    ...
    +  // Get a list of comments containing the link, using db_rewrite_sql() to
    +  // allow comment access modules to exclude comments that the current user
    +  // does not have access to view.
    

    Similar issue here as above, although I'm not 100% sure how things actually work for comment access in Drupal 7... Perhaps it just needs a $query->addTag('node_access') line too? (Since comments have a node.nid column in the database, and this should allow node access modules to exclude comments that are attached to nodes the user doesn't have access to.)

  3. +        if (in_array($field['type'], $fields_supported) && field_access('view', $field, NULL, $node)) {
    

    The third parameter to field_access() should be 'node', not NULL.

  4. +function _linkchecker_link_block_ids($link) {
    ...
    +  $query->condition(db_or()
    +    ->condition('r.rid', $rids, 'IN')
    +    ->condition('r.rid', NULL, 'IS NULL')
    

    Does that last line actually work? Regardless, I think ->isNull(r.rid') is supposed to be the best way to do that.

hass’s picture

Similar issue here as above, although I'm not 100% sure how things actually work for comment access in Drupal 7... Perhaps it just needs a $query->addTag('node_access') line too? (Since comments have a node.nid column in the database, and this should allow node access modules to exclude comments that are attached to nodes the user doesn't have access to.)

I'm not familiar with addTag('node_access') at all and wasn't aware of this. This are not really security bugs to me, I see this more like a missing node_access feature. I have also added addMetaData('base_table', 'comment') per http://drupal.org/node/1204572, but I do not understand how this works internally. This is a major DX failure to name 'node_access', but using taxonomy tables and also write for every node listing query above, but may be a bug in this page!? Than http://drupal.org/node/310077 is also more confusing and also has a entity_field_access (anything required for this?). Based on this I guess http://drupal.org/node/1204572 should use 'term_access' and not 'node_access', but I have no clue and also not how to test all this variants. Who can we ask?

Does that last line actually work? Regardless, I think ->isNull(r.rid') is supposed to be the best way to do that.

Good catch, I wasn't aware of ->isNull(), but this makes sense as this is really not DB independed otherwise.

Aside, not using addTag('translatable') in the node querys sounds like this will solve the i18n issues in D7 by itself and I do not need to do any extra exclusions as required in D6 #1488572: i18n: "Permission restrictions deny" all broken links in other languages.

hass’s picture

hass’s picture

Aside running function _linkchecker_link_access($link) on a block edit (admin/structure/block/manage/block/1/configure) sound pretty useless to me.

First two selects for nothing (possible followup):

SELECT n.nid AS nid FROM node n INNER JOIN linkchecker_node ln ON ln.nid = n.nid WHERE (ln.lid = '1473')
SELECT c.cid AS cid FROM comment c INNER JOIN linkchecker_comment lc ON lc.cid = c.cid WHERE (lc.lid = '1473')
SELECT bid FROM linkchecker_block_custom WHERE lid = '1473'
hass’s picture

hass’s picture

Status: Needs review » Fixed

Committed #13 for now as this issue is blocking me from working on all other issues.

http://drupalcode.org/project/linkchecker.git/commit/cb234f4

hass’s picture

hass’s picture

@David: To fix #1489132: "Permission restrictions deny" message on many broken links when https securepages we need to ignore the protocol in _linkchecker_extract_node_links() at if (strpos($item, $original_link) !== FALSE) {. Do you see a problem with removing the protocol (https?:// or only https?)?

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