Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: External API #1079

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
5 participants
@callumacrae
Copy link
Contributor

commented Nov 12, 2012

This PR contains the beginnings of the external API. There are no tests yet (because phpunit won't work), but I'll be adding some when I've got them working.

Current controller is a test because the external API can't be written until there is an external API (which could take a while). phpBB/api.php/echo/blablabla will output "blablabla" wrapped in either some JSON or XML, depending on what you set the Accept header to.

if (preg_match('/[^a-z0-9]/i', $path[1]) || $path[1] === 'api')
{
exit;

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 12, 2012

Author Contributor

Should probably throw a 404

This comment has been minimized.

Copy link
@michaelcullum

michaelcullum Nov 12, 2012

Member

When we run on controllers this will use the 404 not found exception.

$user->setup();
$format = $request->header('Accept') === 'text/xml' ? 'xml' : 'json';
$path = explode('/', $request->server('PATH_INFO'));

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Nov 12, 2012

Contributor

After the front controller PR is merged, you can use the $symfony_request global variable, which is a Symfony Request object that has a getPathInfo() method. This accounts for when mod_rewrite is used to remove the filename from the url (i.e. ./api/blah/foo instead of ./api.php/blah/foo), whereas PATH_INFO does not exist when that is done normally. It is what is used in the controller stuff for when app.php is removed from the URL.

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Nov 12, 2012

Contributor

In fact, IMO this whole thing could be written as a controller using the route /api/{path}.{type} where type defaults to one of json or xml and path is any string not containing a /.

That way you don't have to have any of this global code. You'd just need to wait until the controller PR is merged.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 12, 2012

Author Contributor

I'm pretty sure that PATH_INFO does still exist when that is done locally, but I see your point.

It was agreed on the RFC that we would not use the type in the URL, but would instead use the Accept header.

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Nov 12, 2012

Contributor

What I meant about path_info not being present is, at least from my experience with the controller PR, when the app.php file was removed from the URL, the server PATH_INFO parameter was not present. When app.php was present in the URL, PATH_INFO was filled. The Symfony Request class has methods to determine the path info even when it is not explicitly provided by the server.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 13, 2012

Author Contributor

Oh, you're correct. I though it remained when it was rewritten.

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

Path info can have backslashes.

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

It was agreed on the RFC that we would not use the type in the URL, but would instead use the Accept header.

You can use accept header as a fallback when format is not specified in the url.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

Please discuss that on the RFC. I already have, if that helps.

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

I read the rfc and I'm clarifying what was decided (as opposed to what you think was decided).

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

Ah, it might have happened in IRC, then. I was specifically told that using the accept header as a fallback is not acceptable.

* @param array $data Array of data to handle.
* @param string $format Format to turn the data into.
*/
protected function handleOutputData($data, $format)

This comment has been minimized.

Copy link
@erikfrerejean

erikfrerejean Nov 14, 2012

Camelcase, should be handle_output_data

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 14, 2012

Author Contributor

But it's PHP, we don't need standardised function names!!!!1

On 14 Nov 2012, at 11:00, Erik Frèrejean notifications@github.com wrote:

In phpBB/includes/api/api.php:

  •   $result = call_user_func_array(array($this, $func_name), $this->args);
    
  •   $this->handleOutputData($result, $format);
    
  •   return true;
    
  • }
  • /**
  • * Handle output data - turn array into string of something like JSON or
  • * XML and then output it.
  • * @todo: Hook for adding other output formats.
  • * @param array $data Array of data to handle.
  • * @param string $format Format to turn the data into.
  • */
  • protected function handleOutputData($data, $format)
    Camelcase, should be handle_output_data


Reply to this email directly or view it on GitHub.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 14, 2012

Author Contributor

I was joking :-)

On 14 Nov 2012, at 11:25, Erik Frèrejean notifications@github.com wrote:

In phpBB/includes/api/api.php:

  •   $result = call_user_func_array(array($this, $func_name), $this->args);
    
  •   $this->handleOutputData($result, $format);
    
  •   return true;
    
  • }


Reply to this email directly or view it on GitHub.

* @param string $method Request method.
* @param array $path_data Array of path data.
* @param string $format Desired output format.
*/

This comment has been minimized.

Copy link
@erikfrerejean

erikfrerejean Nov 14, 2012

@return docblock missing.

*/
public function throw404($message, $format = false)
{
header('HTTP/1.0 404 Not Found');

This comment has been minimized.

Copy link
@erikfrerejean

erikfrerejean Nov 14, 2012

Should probably be calling the send_status_line function here.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 14, 2012

Author Contributor

What's that?

On 14 Nov 2012, at 11:05, Erik Frèrejean notifications@github.com wrote:

In phpBB/includes/api/api.php:

  •   }
    
  •   else
    
  •   {
    
  •       header('Content-type: application/json');
    
  •       echo json_encode($data);
    
  •   }
    
  • }
  • /**
  • * Throws a 404 error in the format used by the API.
  • * @param string $message The message to error with.
  • */
  • public function throw404($message, $format = false)
  • {
  •   header('HTTP/1.0 404 Not Found');
    
    Should probably be calling the send_status_line function here.


Reply to this email directly or view it on GitHub.

This comment has been minimized.

Copy link
@erikfrerejean

erikfrerejean Nov 14, 2012

A function defined in functions.php assuring that the header call is made properly for every instance.

@callumacrae

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2012

I've addressed all the comments so far :-)

@p

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2012

Format should be specifiable via url parameter.

$auth->acl($user->data);
$user->setup();
$format = $request->header('Accept') === 'text/xml' ? 'xml' : 'json';

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

This is a misuse of accept header in any case.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

Please give the RFC a read. You can find it here: http://area51.phpbb.com/phpBB/viewtopic.php?f=108&t=42731

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

Has nothing to do with that. Read http://tools.ietf.org/html/rfc2616. You need a proper parser.

$format = $request->header('Accept') === 'text/xml' ? 'xml' : 'json';
$path = explode('/', $request->server('PATH_INFO'));
if (preg_match('/[^a-z0-9]/i', $path[1]) || $path[1] === 'api')

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

Missing ^ and $.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

Actually… I'm not!

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

There is either insufficient or insufficiently clear validation going on.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

I'll break it down for you:

  • [^...] is a negated character class. It matches characters that aren't in the character class.
  • a-z0-9 means any characters that are letters or numbers.

So… it matches any characters that aren't letters or numbers!

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

You are checking a single character in $path[1], and you should be checking the entire thing against an acceptable regexp.

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

!preg_match('/^[a-z0-9]+$/i', $path[1]) || $path[1] == 'api'

Validation should always be allowing stuff rather than rejecting stuff (whitelisting vs blacklisting). Whenever the code tries to blacklist I assume it's wrong until proven correct (which this code now has been), and even then blacklisting is much easier to screw up unintentionally by someone modifying the code later than whitelisting. Blacklisting is also harder to review as you've just found out.

On which note, an empty $path[1] will pass the test as written in the diff. Which means the code is not in fact correct.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

If the user has a file called ".php" in the api directory, that makes them a stupid user :-D

This comment has been minimized.

Copy link
@p

p Nov 17, 2012

Contributor

"Defense in depth". Look it up.

This comment has been minimized.

Copy link
@michaelcullum

michaelcullum Nov 17, 2012

Member

We will be using symfony controllers and routing anyway? There will be no api directory then (except for classes obviously).

This comment has been minimized.

Copy link
@p

p Nov 17, 2012

Contributor

Then I don't understand why I am being argued with?

$path[1] = substr($path[1], 0, -1);
}
if (is_readable($phpbb_root_path . '/includes/api/' . $path[1] . '.php'))

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

$php_ext.

if (!class_exists($class_name))
{
$api = new phpbb_api;
$api->throw404($user->lang('API_CONTROLLER_NOT_FOUND'), $format);

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

There is no throwing of 404 in http. Please read the http rfc and find the correct terminology to use for referring to returning a response.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

Please just tell me…

On 15 Nov 2012, at 16:43, Oleg Pudeyev wrote:

In phpBB/api.php:

+if (substr($path[1], -1) === 's')
+{

  • $path[1] = substr($path[1], 0, -1);
    +}

+if (is_readable($phpbb_root_path . '/includes/api/' . $path[1] . '.php'))
+{

  • include($phpbb_root_path . '/includes/api/' . $path[1] . '.php');
    +}

+$class_name = 'phpbb_api_' . $path[1];
+if (!class_exists($class_name))
+{

  • $api = new phpbb_api;
  • $api->throw404($user->lang('API_CONTROLLER_NOT_FOUND'), $format);
    There is no throwing of 404 in http. Please read the http rfc and find the correct terminology to use for referring to returning a response.


Reply to this email directly or view it on GitHub.

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

send_404 would be an improvement.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

Alright, thanks :-)

This comment has been minimized.

Copy link
@michaelcullum

michaelcullum Nov 15, 2012

Member

We should use an exception here anyway.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

What do you mean?

This comment has been minimized.

Copy link
@michaelcullum

michaelcullum Nov 15, 2012

Member

We should have an exception for 404s (See http://php.net/manual/en/language.exceptions.php about exceptions). I think controllers add this. cc / @imkingdavid

/**
* A very small library to turn an array into XML.
*
* @param string $body Name of main element.

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

$main_element

}
/**
* A very small library to turn an array into XML.

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

What can be in this array?

protected function toXML($body, $array, $include_header = true)
{
$string = $include_header ? '<?xml version="1.0" encoding="utf-8" ?>' : '';
$string .= '<' . htmlspecialchars($body) . '>';

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

I don't believe <<> is a valid tag in xml. This requires additional validation.

This comment has been minimized.

Copy link
@callumacrae

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

github botched my escaped angle bracket. Point being, what comes out of htmlspecialchars is not necessarily a valid xml tag name.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

That seems like a valid reason to deny any code that uses it, then. What do you suggest I do, manipulate input given to it? The JSON and the XML output should be the same, but one should be XML and the other should be JSON.

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

You should not generate invalid xml. To do this you need to reject input that will result in invalid xml being generated.

array('<'=>'<') is a valid array. What will your function transform it to?

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor

That simply shouldn't be passed to the function. I could also write some code to pass the database credentials to it, but I'm not going to write anything to stop people from doing it!

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor
  1. Which is why I asked you to document what is allowed to be in the array in a preceding comment.
  2. I assume it is fairly straightforward to write a regexp to check whether a string is a valid xml tag name, therefore you should simply do this instead of continuing to argue.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 15, 2012

Author Contributor
  1. I will do… when I have finished writing it >_<
  2. And then what should I do if I find an invalid xml tag name? Either: output invalid XML or: kill the script, outputting nothing but an error. I know which I would prefer.

It's also fairly simple to check whether there are database credentials in there, but I would argue against that, too.

@@ -78,6 +78,8 @@
'ALL_TIMES' => 'All times are <abbr title="%2$s">%1$s</abbr>',
'ALL_TOPICS' => 'All Topics',
'AND' => 'And',
'API_CONTROLLER_NOT_FOUND' => 'Controller not found.',
'API_METHOD_NOT_FOUND' => 'Method not found',

This comment has been minimized.

Copy link
@p

p Nov 15, 2012

Contributor

Missing period.

@p

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2012

In the likely case that symfony has a routing solution, we should probably be using it instead of inventing our own.

@michaelcullum

This comment has been minimized.

Copy link
Member

commented Nov 15, 2012

@p if you read previous comments it will be using the controller anyway.

@p

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2012

This PR is missing a link to RFC.

What use cases are being considered? Echo is not a useful service.

@callumacrae callumacrae deleted the callumacrae:feature/api branch Jun 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.