Problem/Motivation

NodeViewController adds all uri relationships it can find as <link>. But according to http://validator.w3.org/, that is not actually valid.

Also, those links are printed for all users. Shouldn't we at least check access to those routes/urls?

Proposed resolution

Not sure, either drop them or at least add check access?

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#15 access--2406533-15.patch3.63 KBDom.
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,265 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
#11 interdiff.txt1.25 KBolli
#11 2406533-11-access.patch3.65 KBolli
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,904 pass(es), 44 fail(s), and 0 exception(s).
[ View ]
#10 interdiff.txt1.55 KBolli
#10 2406533-10-access.patch2.4 KBolli
#2 node-links-2406533-1-access.patch849 bytesBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,906 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#2 node-links-2406533-1-canonical-only.patch1.59 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,891 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Comments

Berdir’s picture

Issue summary:View changes
Berdir’s picture

Status:Active» Needs review
StatusFileSize
new1.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,891 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
new849 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,906 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Wondering if if we have any tests for this.

Terms have the same logic btw, but not any other entity type, not like this. Just doing node for now.

Wim Leers’s picture

Also, those links are printed for all users. Shouldn't we at least check access to those routes/urls?

Those URLs can easily be determined by malicious users with 99% reliability by looking at the Drupal 8 source code. There's no sensitive information disclosure if those URLs, when accessed by users with insufficient permissions, merely return a 403.

Berdir’s picture

Yes, I'm not saying it is a security issue. But we don't display an edit link in an operations element if you don't have edit permissions either?

Status:Needs review» Needs work

The last submitted patch, 2: node-links-2406533-1-access.patch, failed testing.

The last submitted patch, 2: node-links-2406533-1-canonical-only.patch, failed testing.

Wim Leers’s picture

#4: oh, okay. I think it's fine to show inaccessible links, because indeed we take great care to only show links to the user that will work. But in this case, the user is not going to be human, it's going to be a bot (or at least other software). I think it's fine to simply send all link relationships; it can figure out by doing HTTP HEAD requests whether those links are accessible or not.

Berdir’s picture

Did a bit of research, there are different resources, the validator itself documents the following two lists as his source:

https://html.spec.whatwg.org/multipage/semantics.html#linkTypes
http://microformats.org/wiki/existing-rel-values#HTML5_link_type_extensions

The second is the one that defines canonical and shortlink and a lot of other crap (quoting @WimLeers). But no edit-form or delete-form. It does have a EditUrl, though. Apparently used by wordpress, which is big enough to make its own standards :p

Wim Leers’s picture

From the W3C validator's output:

You can register link types on the http://microformats.org/wiki/existing-rel-values#HTML5_link_type_extensions yourself.

/facepalm


Then, at http://microformats.org/wiki/existing-rel-values#non_HTML_rel_values:

"There are markup languages other than HTML that also have a rel attribute, often based upon the HTML rel attribute. It is useful to document some of these other languages and their rel values for both reference purposes, and to provide background research for the possible development and re-use of these values in HTML, as http://microformats.org/wiki/poshformats or http://microformats.org/wiki/microformats"

[…]

See http://www.iana.org/assignments/link-relations.html for more.

That then lists, amongst others, the edit-form relationship.


Conclusion: this is a fustercluck. So I'd say: let's add all the relationships Drupal uses, reference the IANA URL, and wait for the W3C parser to pick them up, and call it a day.

olli’s picture

Extended 1-access.patch to node preview and term page.

olli’s picture

Status:Needs work» Needs review
StatusFileSize
new3.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,904 pass(es), 44 fail(s), and 0 exception(s).
[ View ]
new1.25 KB

Fixed the failing test.

Status:Needs review» Needs work

The last submitted patch, 11: 2406533-11-access.patch, failed testing.

Dom.’s picture

Dom.’s picture

@Wim Leers about #9: did anyone did that ?

let's add all the relationships Drupal uses, reference the IANA URL, and wait for the W3C parser to pick them up, and call it a day.

Dom.’s picture

Status:Needs work» Needs review
StatusFileSize
new3.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,265 pass(es), 40 fail(s), and 1 exception(s).
[ View ]

Patch #11 does not apply. Just changed it to apply. I did not fix anything more.

Status:Needs review» Needs work

The last submitted patch, 15: access--2406533-15.patch, failed testing.

Wim Leers’s picture

#14: not AFAIK. Pinging @Crell.

Crell’s picture

Huh. I didn't even realize there was a link relation validator...

To understand the intent here, see the issue summary of #2113345: Define a mechanism for custom link relationships (which is postponed to 8.1 for beta-restriction reasons).

In short, if we are going to auto-generate relationships for every ER field, which in the long run I think we will, formally registering those with a standards body is way too involved and they'd probably slap us. Instead, use Curies, which namespaces relations in a sane fashion. We just haven't built out the mechanism for that yet, but if someone wants to then see the linked issue. :-)

Access-restricting the links makes some sense, but honestly I'd ignore the W3C validation for the time being. (Unless someone disagrees with the plan above, in which case let's discuss that.)

Cliff’s picture

The plan—including to ignore the W3C validation for the time being—makes perfect sense.

I am not aware of any values of rel that are used by screen readers or other assistive technologies to offer better accessibility. I can think of ways it could be used, but not of any ways that are recognized and implemented widely.

Wim Leers’s picture

So what are then the concrete next steps here?

Crell’s picture

+++ b/core/modules/node/src/Controller/NodePreviewController.php
@@ -32,6 +32,11 @@ public function view(EntityInterface $node_preview, $view_mode_id = 'full', $lan
+      // Only add links the user is allowed to visit.
+      if (!$node_preview->urlInfo($rel)->access()) {
+        continue;
+      }

So really, the only question is whether we want to do this or not. All of the links exist; all will 403 if accessed by a user who shouldn't view them. But do we omit them or not?

I'm not sure which approach I prefer. I put a question to the HAL-Discuss mailing list, so let's see what they recommend:

https://groups.google.com/forum/#!topic/hal-discuss/nxBWSbPItGA

My knee-jerk inclination if there's no consensus is to show ALL THE LINKS, for greater flexibility and easier implementation, but we'll see if there's a consensus on the list.

mirzu’s picture

I would strongly recommend against putting links into an API response that the user isn't able to use. The only reason to put links at all is to make the Developer Experience better for client developers. This would be like simply showing the whole menu tree to an admin and asking them to click around to find what works and what doesn't.

http://en.wikipedia.org/wiki/HATEOS

The wikipedia example shows an example of a bank account. The API shows a user with money in the bank options like deposit, withdrawal and transfer. The user with 0 balance has only the option to deposit.

I'm sure I can dig up other examples, but imagine how frustrating it would be to use if all users always had all links in the admin menu regardless of permissions.

Crell’s picture

mirzu: Well, the implications of that include:

1) We can't rely on HTTP caching for non-anonymous, because we would need to make REST responses user.permissions cache context aware. All requests still will hit Drupal to check that.
2) Both in REST and in HTML Link tags, we'd need to include permission checks. Doable, but more work, and more things for us to potentially get wrong. :-) We'd want to make sure it can be addressed globally.

Berdir’s picture

1) We need to do that anyway. The output is already permission specific, you might not see some fields based on your permissions.

mirzu’s picture

1) The anonymous API user is the same as the anonymous HTML user. We can add edit links to pages if a user is logged in, and omit those links when it's served to an anonymous user. This is the same idea. Anonymous output is cachable, user specific output isn't. The links for the HTML user and the API user analogous here. IMO.

I did a quick poke around a few APIs and can confirm that it's really frustrating when trying to "discover" available resources and 90% of links lead to a 403.

Crell’s picture

Berdir: Hm. That's a very valid point, so destroys my point 1.

Which leaves only point 2, "if we're going to do it, don't do it ad hoc". We want to (in 8.1 if not sooner) auto-adding as many relevant links as we can to both HTML head and HAL links. If we're going to filter for access control, we should make sure that is done in a consistent and clean fashion for links, rather than one-off in each case, if possible. That or confirm that there's only 2-3 places it even matters, so we don't need to worry about it. For the latter to work, we would need to centralize the link-addition out of NodeController and into generic Entity handling, which frankly is something we should have done a long time ago anyway.

Perhaps the one-off (patches above or similar) for now with @todo statements, and open an 8.1 issue to centralize/automate those, which should simplify a lot of code anyway (and is useful in its own right independently of anything we're discussing here)?

Wim Leers’s picture

1. What if the access result depends on more than just permissions? What if it is per user too? Or worse?

Then we'd be destroying cacheability for the sake of a tiny, tiny, tiny fraction of user agents that actually use this information, that's not even visible to human users.

Let's not forget about that.