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
Comment | File | Size | Author |
---|---|---|---|
#27 | 2348007-27.patch | 6.25 KB | jibran |
#27 | interdiff.txt | 822 bytes | jibran |
Comments
Comment #1
catchiirc the taxonomy_index table only has published nodes in it?
Comment #2
jibranThis 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.
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
Comment #3
olli CreditAttribution: olli commentedIt 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.
Comment #8
jibranI am fine by this change.
But I don't think we need this because of the reasons stated above. @dawehner any comments?
Comment #10
Fabianx CreditAttribution: Fabianx commentedSetting 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 ...
Comment #11
jibranWe need @dawehner approval here because he is views maintainer and it in not showing unpublished items please see #2.
Comment #12
jibranWe still don't need a status filter. Here is the proof.
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.
Comment #13
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #14
dawehnerSo 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_index
table,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.
Comment #15
catchShouldn't that be a 404?
If we do that, we should denormalize the published column to {taxonomy_index} so the condition can be applied on that table.
Comment #16
jibran@catch
It is 404 now(not in the head though) until you set 'all' as an exception value. See patch in #3.
{taxonomy_index} has only publish node ids in it but yeah we can add published column to {taxonomy_index}.
@dawehner
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.
Comment #17
marcus7777 CreditAttribution: marcus7777 commentedbut the unpublished nodes are still visible.
before and after:
Comment #18
jibran#17 you have to re-import the view(views.view.taxonomy_term.yml).
Comment #19
dawehnerOkay, let's go with #3 and continue to work somewhere else!
Comment #21
jibranReroll of #3 and RTBC as per #19.
Comment #22
catchThis 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.
Comment #23
jibranI 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.
Comment #24
jibranHere 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.
Comment #25
larowlanlooks ok to me
Comment #26
larowlanActually - we don't need the update hook yet
Comment #27
jibranOk cool. Thanks for the review.
Comment #28
olli CreditAttribution: olli commentedThere's also a condition on node_field_data.langcode. Does #22 mean we should also move that to taxonomy_index?
Comment #29
amateescu CreditAttribution: amateescu commented@catch, the query for this view is already bad in HEAD:
And after adding the node status filter:
This is on a test site with 1054 nodes and 2 terms.
Comment #30
catchHow come we're querying on current language on monolingual sites?
Comment #31
amateescu CreditAttribution: amateescu commentedThat's exactly what I was wondering as well :)
Comment #32
jibranFor #30 and #31 see #2337509: Remove "@todo In theory we should use the data table as base table, as this would" from EntityViewsData.
Comment #33
jibranComment #34
dawehnerLet's split that up into a separate task first: #2375879: Don't filter languages in case it is not needed.
Comment #35
Fabianx CreditAttribution: Fabianx commentedRTBC, might need a re-roll once #2375879: Don't filter languages in case it is not needed. is in.
Comment #36
alexpotttaxonomy_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!Comment #38
jibranCreated #2384583: Remove taxonomy_select_nodes function for #36 please review.
Comment #39
jibranBTW this the query after #2375879: Don't filter languages in case it is not needed.