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.

CommentFileSizeAuthor
#12 openid-fragment-normalization-D6-1.patch869 bytesc960657
#10 openid-fragment-normalization-3.patch7.34 KBc960657
#7 do575805-openid-fragment-normalization.patch2.61 KBheine
#5 do575805-openid-fragment-normalization.patch2.64 KBheine
#4 do575805-openid-fragment-normalization.patch2.11 KBheine
openid_spec_violation_fragment_identifier.patch724 bytesheine

Comments

heine’s picture

heine commented
Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

Anyone willing to supply a test?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: +Needs tests

Tagging.

heine’s picture

heine commented

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...

heine’s picture

heine commented
Status: Needs work » Needs review
StatusFileSize
new do575805-openid-fragment-normalization.patch2.11 KB

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.

heine’s picture

heine commented
StatusFileSize
new do575805-openid-fragment-normalization.patch2.64 KB

Added missing hunk.

c960657’s picture

c960657 commented

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

heine’s picture

heine commented
StatusFileSize
new do575805-openid-fragment-normalization.patch2.61 KB

Added periods.

c960657’s picture

c960657 commented 
+    $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)

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

c960657 commented
Status: Needs work » Needs review
StatusFileSize
new openid-fragment-normalization-3.patch7.34 KB
dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS. Thanks for the tests.

c960657’s picture

c960657 commented
Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs review
StatusFileSize
new openid-fragment-normalization-D6-1.patch869 bytes

D6 backport.

alex_b’s picture

alex_b commented
Status: Needs review » Reviewed & tested by the community

Tested, works as expected. RTBC

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

pwolanin’s picture

pwolanin commented

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

c960657’s picture

c960657 commented
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.