Problem/Motivation

Default taxonomy term view does not have "Publishing status" filter but accepts an exception value 'all' for "Has taxonomy term". With exception value in URL, the page display returns 404 (this may be a separate bug) but the feed display correctly ignores the argument and displays all results including unpublished nodes.

Steps to reproduce: Install, create an unpublished article and visit /taxonomy/term/all/feed.

Proposed resolution

Add the status filter or remove exception value or both.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

iirc the taxonomy_index table only has published nodes in it?

jibran’s picture

Status: Active » Closed (works as designed)

This is correct that we don't have publish status check in taxonomy term view and this is also correct we only store published nids in taxonomy_index.

function taxonomy_build_node_index($node) {
  // We maintain a denormalized table of term/node relationships, containing
  // only data for current, published nodes.
  if (!\Drupal::config('taxonomy.settings')->get('maintain_index_table') || !(\Drupal::entityManager()->getStorage('node') instanceof SqlContentEntityStorage)) {
    return;
  }

  $status = $node->isPublished();
  $sticky = (int) $node->isSticky();
  // We only maintain the taxonomy index for published nodes.
  if ($status && $node->isDefaultRevision()) {
    // Collect a unique list of all the term IDs from all node fields.
    $tid_all = array();
    foreach ($node->getFieldDefinitions() as $field) {
      $field_name = $field->getName();
      if ($field->getType() == 'taxonomy_term_reference') {
        foreach ($node->getTranslationLanguages() as $language) {
          foreach ($node->getTranslation($language->id)->$field_name as $item) {
            if (!$item->isEmpty()) {
              $tid_all[$item->target_id] = $item->target_id;
            }
          }
        }
      }
    }
    // Insert index entries for all the node's terms.
    if (!empty($tid_all)) {
      foreach ($tid_all as $tid) {
        db_merge('taxonomy_index')
          ->key(array('nid' => $node->id(), 'tid' => $tid))
          ->fields(array('sticky' => $sticky, 'created' => $node->getCreatedTime()))
          ->execute();
      }
    }
  }
}

There was a security vulnerability in D7 core https://www.drupal.org/SA-CORE-2014-001 but it was fixed and for D8 we don't have that vulnerability anymore see #2174479: Forward-port tests for taxonomy upgrade portion of SA-CORE-2014-001 to Drupal 8

olli’s picture

Category: Task » Bug report
Issue summary: View changes
Status: Closed (works as designed) » Needs review
FileSize
1.61 KB
3.49 KB

It seems views makes an outer join when the argument is ignored (at /taxonomy/term/all/feed). Added steps to issue summary.

Here's a patch that does both: removes 'all' and adds status filter.

Status: Needs review » Needs work

The last submitted patch, 3: 2348007-3.patch, failed testing.

The last submitted patch, 3: 2348007-fail.patch, failed testing.

jibran queued 3: 2348007-fail.patch for re-testing.

Status: Needs work » Needs review

jibran queued 3: 2348007-3.patch for re-testing.

jibran’s picture

  1. +++ b/core/modules/taxonomy/config/install/views.view.taxonomy_term.yml
    @@ -105,9 +105,9 @@ display:
    +          default_action: 'not found'
    

    I am fine by this change.

  2. +++ b/core/modules/taxonomy/config/install/views.view.taxonomy_term.yml
    @@ -183,6 +183,46 @@ display:
    +        status:
    

    But I don't think we need this because of the reasons stated above. @dawehner any comments?

The last submitted patch, 3: 2348007-fail.patch, failed testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Security improvements

Setting to RTBC to get committer feedback.

I think not having a published check and showing unpublished items is an information disclosure bug?

The test failures show at least the bugs nicely ...

jibran’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Security improvements

We need @dawehner approval here because he is views maintainer and it in not showing unpublished items please see #2.

jibran’s picture

FileSize
2.19 KB
1.75 KB

We still don't need a status filter. Here is the proof.

It seems views makes an outer join when the argument is ignored (at /taxonomy/term/all/feed). Added steps to issue summary.

This is correct but this not because of taxonomy view it is by default in views so it is a separate bug as stated in issue summary.

Fabianx’s picture

Issue tags: +Security improvements

I am sorry, but if the test shows that an unpublished item is in the feed, that is an information disclosure security bug.

I don't think changing tests is the solution to that. I am also sorry if I misunderstand the issue at large, but I will seek dawehner to gain an understanding of it.

Can we please keep the 'Security improvements' tag for now until it was verified by someone not me or you that this is indeed not a security problem?

I will bug dawehner today about this.

Thanks for your efforts and the patches.

dawehner’s picture

So yeah even taxonomy_index does not contain any unpublished nodes,
the case in which you do pass invalid terms / pass in 'all' you no longer query against the taxonomy_indextable,
so that you result in unpublished nodes ...

On top of that the 'node_access' query tag is applied, but that one does not securer you against unpublished nodes by default.

TL;DR

Afaik we want to add the node: published filter, if you ask me.

catch’s picture

the case in which you do pass invalid terms / pass in 'all' you no longer query against the taxonomy_indextable,
so that you result in unpublished nodes ...

Shouldn't that be a 404?

Afaik we want to add the node: published filter, if you ask me.

If we do that, we should denormalize the published column to {taxonomy_index} so the condition can be applied on that table.

jibran’s picture

@catch

Shouldn't that be a 404?

It is 404 now(not in the head though) until you set 'all' as an exception value. See patch in #3.

If we do that, we should denormalize the published column to {taxonomy_index} so the condition can be applied on that table.

{taxonomy_index} has only publish node ids in it but yeah we can add published column to {taxonomy_index}.
@dawehner

Afaik we want to add the node: published filter, if you ask me.

Then the fix in #3 is fine and patch is RTBC.
@Fabianx
I am sorry if you find my behavior annoying in #12. But yes you are right in your observation. Thanks for understanding.

marcus7777’s picture

Status: Needs review » Needs work
FileSize
171.13 KB
$ git apply --index 2348007-11.patch
$ drush cr

but the unpublished nodes are still visible.

before and after:

jibran’s picture

Status: Needs work » Needs review

#17 you have to re-import the view(views.view.taxonomy_term.yml).

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Okay, let's go with #3 and continue to work somewhere else!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2348007-11.patch, failed testing.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.42 KB

Reroll of #3 and RTBC as per #19.

catch’s picture

Status: Reviewed & tested by the community » Needs work

This needs an EXPLAIN on the query - given it adds a condition on a different table to the one with the sort, it is probably going to be quite bad.

We should probably add a status column to the term_index table.

jibran’s picture

I am +1 on adding a new column to the term_index table. I'll write the patch over the weekend if someone doesn't beat me to it.

jibran’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
7.05 KB

Here is the first attempt. I have created an update hook to update the schema but I think entity storage also has an schema update event which is throwing exception on update.

larowlan’s picture

looks ok to me

larowlan’s picture

Actually - we don't need the update hook yet

jibran’s picture

FileSize
822 bytes
6.25 KB

Ok cool. Thanks for the review.

olli’s picture

There's also a condition on node_field_data.langcode. Does #22 mean we should also move that to taxonomy_index?

amateescu’s picture

@catch, the query for this view is already bad in HEAD:

SELECT taxonomy_index.sticky AS taxonomy_index_sticky, taxonomy_index.created AS taxonomy_index_created, node.nid AS nid, node_field_data.langcode AS node_field_data_langcode
FROM 
{node} node
INNER JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
LEFT JOIN {taxonomy_index} taxonomy_index ON node.nid = taxonomy_index.nid
WHERE (( (node_field_data.langcode IN  ('en')) ))
ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC
LIMIT 11 OFFSET 0
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE node index PRIMARY node__vid 5 NULL 1011 Using index; Using temporary; Using filesort
1 SIMPLE node_field_data eq_ref PRIMARY, node_field__langcode PRIMARY 42 d8_dev.node.nid,const 1 Using where; Using index
1 SIMPLE taxonomy_index ref PRIMARY PRIMARY 4 d8_dev.node.nid 1

And after adding the node status filter:

SELECT taxonomy_index.sticky AS taxonomy_index_sticky, taxonomy_index.created AS taxonomy_index_created, node.nid AS nid, node_field_data.langcode AS node_field_data_langcode
FROM 
{node} node
INNER JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
LEFT JOIN {taxonomy_index} taxonomy_index ON node.nid = taxonomy_index.nid
WHERE (( (node_field_data.langcode IN  ('en')) AND (node_field_data.status = '1') ))
ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC
LIMIT 11 OFFSET 0
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE node_field_data ref PRIMARY, node_field__langcode, node__status_type node__status_type 1 const 1051 Using where; Using index; Using temporary; Using filesort
1 SIMPLE node eq_ref PRIMARY PRIMARY 4 d8_dev.node_field_data.nid 1 Using index
1 SIMPLE taxonomy_index ref PRIMARY PRIMARY 4 d8_dev.node.nid 1

This is on a test site with 1054 nodes and 2 terms.

catch’s picture

How come we're querying on current language on monolingual sites?

amateescu’s picture

That's exactly what I was wondering as well :)

jibran’s picture

jibran’s picture

dawehner’s picture

Let's split that up into a separate task first: #2375879: Don't filter languages in case it is not needed.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, might need a re-roll once #2375879: Don't filter languages in case it is not needed. is in.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

taxonomy_select_nodes() has no usages - we should (in a followup) either write a test for it or remove it. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9ae8e2c and pushed to 8.0.x. Thanks!

  • alexpott committed 9ae8e2c on 8.0.x
    Issue #2348007 by jibran, olli, marcus7777: Taxonomy term view needs...
jibran’s picture

jibran’s picture

BTW this the query after #2375879: Don't filter languages in case it is not needed.

EXPLAIN SELECT taxonomy_index.sticky AS taxonomy_index_sticky, taxonomy_index.created AS taxonomy_index_created, node.nid AS nid
    -> FROM 
    -> node node
    -> LEFT JOIN taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid
    -> WHERE (( (taxonomy_index.tid = '1') )AND(( (taxonomy_index.status <> '0') )))
    -> ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC
    -> LIMIT 11 OFFSET 0;
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE node index PRIMARY node__vid 5 NULL 1 Using index; Using temporary; Using filesort
1 SIMPLE taxonomy_index eq_ref PRIMARY,term_node PRIMARY 8 d8.node.nid,const 1 Using where

Status: Fixed » Closed (fixed)

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