Download & Extend
OpenID » Issues

OpenID URL normalization #fragment handling spec violation

Posted by Heine on September 12, 2009 at 11:47pm
Project:OpenID
Version:5.x-1.x-dev
Component:OpenID Client
Category:bug report
Priority:normal
Assigned:Unassigned
Status:needs review
Issue tags:Needs tests

Issue Summary

The OpenID 2.0 specification states on normalizing user input:

If the URL contains a fragment part, it MUST be stripped off together with the fragment delimiter character "#". See Section 11.5.2 for more information.

The current openid implementation doesn't do this.

AttachmentSize
openid_spec_violation_fragment_identifier.patch724 bytes
Login or register to post comments

Comments

#1

Posted by Heine on September 14, 2009 at 9:09pm
Version:6.x-dev» 7.x-dev
Status:needs review» needs work

Anyone willing to supply a test?

#2

Posted by webchick on September 14, 2009 at 9:25pm

Tagging.

#3

Posted by Heine on September 15, 2009 at 8:44pm

Well, we could go the full monty and implement normalization according to RCF 3986 section 6 as we should.

Inspiration:
http://pear.php.net/package/Net_URL2/docs/latest/__filesource/fsource_Ne...
http://framework.zend.com/svn/framework/standard/trunk/library/Zend/Open...

#4

Posted by Heine on September 15, 2009 at 9:44pm
Status:needs work» needs review

Let's leave the full RFC implementation to #578464: OpenID 2.0 spec violation - Normalize URL according to RFC3986 Section 6. Here's a patch for the fragment issue with a few tests.

AttachmentSize
do575805-openid-fragment-normalization.patch 2.11 KB

#5

Posted by Heine on September 15, 2009 at 9:46pm

Added missing hunk.

AttachmentSize
do575805-openid-fragment-normalization.patch 2.64 KB

#6

Posted by c960657 on September 16, 2009 at 8:53pm

The two last tests miss an ending period in the message. Apart from that it looks good.

#7

Posted by Heine on September 17, 2009 at 7:56am

Added periods.

AttachmentSize
do575805-openid-fragment-normalization.patch 2.61 KB

#8

Posted by c960657 on September 17, 2009 at 5:33pm

+    $this->assertEqual(_openid_normalize('$foo'), '$foo', t('_openid_normalize correctly normalized an XRI.'));

Another nit (sorry): I think we usually refer to functions using brackets, i.e. _openid_normalize() correctly normalized an XRI., though the other tests in that file don't follow that convention.

(I think we generally need some guidelines on how to phrase the assertion message for tests)

#9

Posted by System Message on September 24, 2009 at 10:10am
Status:needs review» needs work

The last submitted patch failed testing.

#10

Posted by c960657 on November 23, 2009 at 10:36pm
Status:needs work» needs review
AttachmentSize
openid-fragment-normalization-3.patch 7.34 KB

#11

Posted by Dries on November 24, 2009 at 5:21am
Status:needs review» fixed

Committed to CVS. Thanks for the tests.

#12

Posted by c960657 on November 24, 2009 at 7:32pm
Version:7.x-dev» 6.x-dev
Status:fixed» needs review

D6 backport.

AttachmentSize
openid-fragment-normalization-D6-1.patch 869 bytes

#13

Posted by alex_b on January 12, 2010 at 7:29pm
Status:needs review» reviewed & tested by the community

Tested, works as expected. RTBC

#14

Posted by Gábor Hojtsy on March 1, 2010 at 11:06am
Status:reviewed & tested by the community» fixed

Thanks, committed.

#15

Posted by pwolanin on March 2, 2010 at 5:29pm

Does this need to be backported to the D5 contrib module?

#16

Posted by c960657 on March 3, 2010 at 9:34pm
Project:Drupal core» OpenID
Version:6.x-dev» 5.x-1.x-dev
Component:openid.module» OpenID Client
Status:fixed» needs review

Yes, I think a backport for D5 is relevant. The D6 patch applies to the D5 contrib module with just a small offset.