Ticket #5298 (closed defect: fixed)

Opened 10 months ago

Last modified 10 months ago

https atom service document doesn't point to https collections

Reported by: rubys Assigned to: westi
Priority: normal Milestone: 2.5
Component: General Version: 2.3.1
Severity: normal Keywords: has-patch
Cc: josephscott, rubys

Description

It turns out that specifying https://<your_blog_here>.wordpress.com/wp-app.php doesn't actually get you a secure connection... all it does it retrieves a relatively static service document securely. Unfortunately, that document points you to unsecured collections and category documents.

The fix is relatively straightforward, and attached as a patch. But there is a second problem in that users of RSD will never discover the secure connection (unlike xml-rpc, supporting https is a requirement for AtomPub?). This is also included in the patch.

Finally, there is a small issue with the RSD in that Atom doesn't have a notion of a "blogID", and the definition of RSD indicates that blogID should be "" in such circumstances. That, too, is included in the patch.

Attachments

https-app.patch (4.3 kB) - added by rubys on 11/01/07 11:37:29.
Original patch did not check for https correctly in the RSD
5298.diff (5.4 kB) - added by westi on 11/06/07 19:30:23.
Alternative patch which moves the code around a bit
5298.patch (5.4 kB) - added by rubys on 11/06/07 19:59:55.
reworked and tested patch

Change History

11/01/07 02:47:37 changed by josephscott

  • cc set to josephscott.

11/01/07 03:36:03 changed by foolswisdom

  • keywords set to has-patch.
  • version set to 2.3.1.
  • milestone changed from 2.5 to 2.4.

11/01/07 11:37:29 changed by rubys

  • attachment https-app.patch added.

Original patch did not check for https correctly in the RSD

11/02/07 10:14:03 changed by westi

  • owner changed from anonymous to westi.
  • status changed from new to assigned.

11/03/07 16:20:58 changed by westi

Not quite sure this patch is right at the moment.

Is doing the callback https check on every request to xmlrpc.php really a good idea? Would we not do better to just make sure if you access via https then you stay on https by basing the returned urls on the request url?

(follow-up: ↓ 6 ) 11/04/07 15:49:22 changed by rubys

  • cc changed from josephscott to josephscott, rubys.

Am I misreading line 20 in xmlrpc.php incorrectly? The intent of this patch is to only do this check when fetching the rsd document.

See this post for background. Some place in the traversal from http://foo.wordpress.com/ => xmlrpc.php?rsd => /wp-app.php/service => /wp-app.php/posts or /wp-app.php/attachments there needs to be an indication that https should be used. Doing this when rendering the front-page would clearly be overkill. Doing it on every fetch of the service document would be better. Doing it only when fetching the discovery document seems (to me at least) to be better yet.

(in reply to: ↑ 5 ) 11/06/07 19:12:22 changed by westi

Replying to rubys:

Am I misreading line 20 in xmlrpc.php incorrectly? The intent of this patch is to only do this check when fetching the rsd document. See this post for background. Some place in the traversal from http://foo.wordpress.com/ => xmlrpc.php?rsd => /wp-app.php/service => /wp-app.php/posts or /wp-app.php/attachments there needs to be an indication that https should be used. Doing this when rendering the front-page would clearly be overkill. Doing it on every fetch of the service document would be better. Doing it only when fetching the discovery document seems (to me at least) to be better yet.

Sorry - I read the patch too quick :-( Indeed this does only apply when we are returning the rsd.

I think i would be happier if we moved the check for https availablity out to a generic function and then just had an apply_filter call in xmlrpc.php so that a plugin could force a https url without the run-time cost of the check if the admin knows that https is available for accessing the blog.

11/06/07 19:30:23 changed by westi

  • attachment 5298.diff added.

Alternative patch which moves the code around a bit

(follow-up: ↓ 8 ) 11/06/07 19:30:49 changed by westi

Could you try that new untested patch on for size?

11/06/07 19:59:55 changed by rubys

  • attachment 5298.patch added.

reworked and tested patch

(in reply to: ↑ 7 ; follow-up: ↓ 9 ) 11/06/07 20:06:00 changed by rubys

Replying to westi:

Could you try that new untested patch on for size?

A number of minor issues fixed (unmatched parens in xmlrpc.php, get_bloginfo does an implicit echo, apply_filters does not do an implicit echo).

One substantive change: blog_is_accessable_via_ssl() => url_is_accessable_via_ssl($url). Two reasons for this (1) on my test site https://rubix.home/wordpress (note no trailing slash) returns a 404, and (2) this given the potential plugin the full url and complete freedom to modify any or all of it.

P.S. s/accessable/accessible/ ?

(in reply to: ↑ 8 ) 11/06/07 21:25:39 changed by westi

Replying to rubys:

Replying to westi:

Could you try that new untested patch on for size?

A number of minor issues fixed (unmatched parens in xmlrpc.php, get_bloginfo does an implicit echo, apply_filters does not do an implicit echo). One substantive change: blog_is_accessable_via_ssl() => url_is_accessable_via_ssl($url). Two reasons for this (1) on my test site https://rubix.home/wordpress (note no trailing slash) returns a 404, and (2) this given the potential plugin the full url and complete freedom to modify any or all of it. P.S. s/accessable/accessible/ ?

Thanks for the test and update - I had to run and have dinner so I chucked up my in progress code! I'll have a look at getting this checked in tomorrow

11/17/07 11:21:35 changed by westi

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [6339]) Ensure that we offer https access to atom if it is available. Fixes #5298 props rubys.

(follow-up: ↓ 12 ) 11/17/07 11:27:31 changed by westi

Didn't work for non-SSL :-( see [6340] for fix.

(in reply to: ↑ 11 ) 11/17/07 17:58:27 changed by rubys

Replying to westi:

Didn't work for non-SSL :-( see [6340] for fix.

Good catch! Thanks!