Improve robustness of OpenID parsing for link 'rel' and meta 'http-equiv' attributes

Would it be better to parse the page using DOMDocument::loadHTML() and then find the elements by traversing the DOM tree? Parsing HTML using regular expressions is generally not as easy as it may seem.

is the 6.x version failing because the patch is marked as applying to 7? Just tried it on the current DRUPAL-6 & it applies properly.

Just tried applying vs. HEAD, and it goes smoothly. All tests are passing for me, with 0 exceptions. Perhaps something elsewhere in the codebase was causing the exceptions when this was tested in June?

This patch uses DOMDocument::loadHTML() for parsing the HTML. Parsing HTML using regular expressions is generally a bad idea.

Reroll.

The problem with the first drupal 6.x patch is that the source file name is incorrect. Applying the hereby attached patch should work correctly and fix problems with the community id openid link provided.

The patch I just uploaded shouldn't be tested against the latest revision from CVS. So I'm not surprised it doesn't pass. It's a drupal 6 patch, HEAD is drupal 7. I created a new issue for the 6.x branch: http://drupal.org/node/865098

For now the D7 patch in #9 is the one up for review.

#9: openid-parse-html-2.patch queued for re-testing.

No progress after 2010? D7 and D6 without this patch still fail on my CommunityID provider

Rolled new patch with git. Applies to D7 and D8.

+++ b/core/modules/openid/openid.inc
@@ -371,24 +371,32 @@ function _openid_nonce() {
+    foreach ($html_element->head->link as $link) {
...
+    foreach ($html_element->head->meta as $meta) {

If the DOM unexpectedly does not contain a HEAD or LINK tags, this will trigger notices. Let's add a !empty() condition here before attempting to iterate.

+++ b/core/modules/openid/openid.inc
@@ -371,24 +371,32 @@ function _openid_nonce() {
+      if (preg_match('/(\s|^)' . $rel . '(\s|$)/i', $link['rel'])) {
+        return trim($link['href']);

Missing preg_quote() to $rel. Note that the second argument to it should always be specified ('/' in this case).

For the actual regex, I believe you rather want:

@^(?:\s*)?([rel])(?:\s*)?$@/i

Lastly, I'd rather return the more explicit $matches[1].

9 days to next Drupal core point release.

Let's add a !empty() condition here before attempting to iterate.

Good point. I added an isset() (it seems that empty() checks for text content inside the element).

For the actual regex, I believe you rather want:
@^(?:\s*)?([rel])(?:\s*)?$@/i

No (if I understand you correctly). The rel attribute may contain a space-separated list of link types (see the HTML5 spec), so if rel is e.g. "foo", it should match <link rel="foo bar">. That is not supported by your pattern, right? But I did add the ?:.

Lastly, I'd rather return the more explicit $matches[1].

I'm not sure what you mean? The regexp operates on the rel attribute, but we return the href attribute. The only $matches[1] that appear in the patch is the old code that is removed.

Thanks for clarifying and adjusting!

Looks ready to fly for me.

Committed/pushed to 8.x.

Moving to CNR against Drupal 7 since there's already a patch. Reports above suggest this is a bug rather than a task.

Reuploaded latest D7 for the sake of the test bot.

I wonder if this will also fix the issue where `rel="OLDopenid.server"' (where there are other characters before the "openid.server" string) is positively matched by the _openid_link_href() function on D7...? It seems like it will, but I haven't seen this issue reported anywhere else.

@mathieu: I believe it will. Feel free to test and review the patch, and then mark it “Reviewed and tested by the community” if everything looks fine.