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.

Sign up for GitHub

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

Jump to bottom

WIP: Add support for running raw SQL files #729

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

WIP: Add support for running raw SQL files #729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c3553a
wip: Add support for running raw SQL files
rolandboon Nov 23, 2020
0783de5
Add input argument handling, template parsing and http verbs for SQL …
rolandboon Nov 25, 2020
46c6572
Refactor for PR feedback
rolandboon Dec 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Tqdev/PhpCrudApi/Api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Tqdev\PhpCrudApi\Controller\GeoJsonController;
use Tqdev\PhpCrudApi\Controller\JsonResponder;
use Tqdev\PhpCrudApi\Controller\OpenApiController;
use Tqdev\PhpCrudApi\Controller\ProcedureController;
use Tqdev\PhpCrudApi\Controller\RecordController;
use Tqdev\PhpCrudApi\Database\GenericDB;
use Tqdev\PhpCrudApi\GeoJson\GeoJsonService;
Expand All @@ -37,6 +38,7 @@
use Tqdev\PhpCrudApi\OpenApi\OpenApiService;
use Tqdev\PhpCrudApi\Record\ErrorCode;
use Tqdev\PhpCrudApi\Record\RecordService;
use Tqdev\PhpCrudApi\Procedure\ProcedureService;
use Tqdev\PhpCrudApi\ResponseUtils;

class Api implements RequestHandlerInterface
Expand Down Expand Up @@ -138,6 +140,10 @@ public function __construct(Config $config)
$geoJson = new GeoJsonService($reflection, $records);
new GeoJsonController($router, $responder, $geoJson);
break;
case 'procedures':
$procedures = new ProcedureService($db, $config->getProcedurePath());
new ProcedureController($router, $responder, $procedures);
break;
}
}
foreach ($config->getCustomControllers() as $className) {
Expand Down
8 changes: 7 additions & 1 deletion src/Tqdev/PhpCrudApi/Config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class Config
'database' => null,
'tables' => '',
'middlewares' => 'cors,errors',
'controllers' => 'records,geojson,openapi',
'controllers' => 'records,geojson,procedures,openapi',
'customControllers' => '',
'customOpenApiBuilders' => '',
'cacheType' => 'TempFile',
Expand All @@ -22,6 +22,7 @@ class Config
'debug' => false,
'basePath' => '',
'openApiBase' => '{"info":{"title":"PHP-CRUD-API","version":"1.0.0"}}',
'procedurePath' => 'procedures'
];

private function getDefaultDriver(array $values): string
Expand Down Expand Up @@ -202,4 +203,9 @@ public function getOpenApiBase(): array
{
return json_decode($this->values['openApiBase'], true);
}

public function getProcedurePath(): string
{
return $this->values['procedurePath'];
}
}
42 changes: 42 additions & 0 deletions src/Tqdev/PhpCrudApi/Controller/ProcedureController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php

namespace Tqdev\PhpCrudApi\Controller;

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Tqdev\PhpCrudApi\Middleware\Router\Router;
use Tqdev\PhpCrudApi\Record\ErrorCode;
use Tqdev\PhpCrudApi\Procedure\ProcedureService;
use Tqdev\PhpCrudApi\RequestUtils;

class ProcedureController
{
private $service;
private $responder;

public function __construct(Router $router, Responder $responder, ProcedureService $service)
{
$router->register('GET', '/procedures/*', array($this, 'file'));
$router->register('POST', '/procedures/*', array($this, 'file'));
$router->register('PUT', '/procedures/*', array($this, 'file'));
$router->register('DELETE', '/procedures/*', array($this, 'file'));
$this->service = $service;
$this->responder = $responder;
}

public function file(ServerRequestInterface $request): ResponseInterface
{
$file = RequestUtils::getPathSegment($request, 2);
$operation = RequestUtils::getOperation($request);
$queryParams = array_map(function($param) {
return $param[0];
}, RequestUtils::getParams($request));
$bodyParams = (array) $request->getParsedBody();
$params = array_merge($queryParams, $bodyParams);
if (!$this->service->hasProcedure($file, $operation)) {
return $this->responder->error(ErrorCode::PROCEDURE_NOT_FOUND, $file);
}
return $this->responder->success($this->service->execute($file, $operation, $params));
}
}

14 changes: 14 additions & 0 deletions src/Tqdev/PhpCrudApi/Database/GenericDB.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,20 @@ public function incrementSingle(ReflectedTable $table, array $columnValues, stri
return $stmt->rowCount();
}

public function rawSql(string $sql, array $parameters)
{
$stmt = $this->query($sql, $parameters);
$stmt = $this->pdo->prepare($sql);
foreach ($parameters as $key => $value) {
if (strstr($sql, ':' . $key)) {
$stmt->bindParam(':' . $key, $value, \PDO::PARAM_STR);
}
}
$stmt->execute();
$records = $stmt->fetchAll();
return $records;
}

private function query(string $sql, array $parameters): \PDOStatement
{
$stmt = $this->pdo->prepare($sql);
Expand Down
32 changes: 32 additions & 0 deletions src/Tqdev/PhpCrudApi/Procedure/ProcedureService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace Tqdev\PhpCrudApi\Procedure;

use Tqdev\PhpCrudApi\Database\GenericDB;

class ProcedureService {
private $db;
private $procedurePath;

public function __construct(GenericDB $db, string $procedurePath)
{
$this->db = $db;
$this->procedurePath = $procedurePath;
}

public function hasProcedure(string $procedureName, string $operation) {
return file_exists('./' . $this->procedurePath . '/' . $procedureName . '.' .$operation . '.sql');
}

public function execute(string $procedureName, string $operation, array $params = []) {
$sql = $this->parseSqlTemplate($this->procedurePath . '/' . $procedureName . '.' . $operation . '.sql', $params);
return $this->db->rawSql($sql, $params);
}

private function parseSqlTemplate(string $path, array $context) {
ob_start();
extract($context);
include($path);
Copy link
Owner

@mevdschee mevdschee Nov 28, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to split the file into a head with some meta information and a body containing the sql query. The include is a risk as the $path variable should not contain (unchecked) user input to avoid path traversal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think path traversing is an issue at this point, because the path is limited to a single URL path segment. The value of $path at this location is defined as follows:

$path = './' . Config->getProcedurePath() . '/'. RequestUtils::getPathSegment($request, 2) . '.' . $operation . '.sql';

The getPathSegment() blocks path traversing.

However, some meta info would allow us to handle user input and server responses better. We could define the files like this:

./procedures/example.GET.php

<?php
$procedure = [
	/*
	 * Define user input from path
	 * In this example the route will be GET /procedure/example/:id
	 * The value of :id will be as :id in the PDO statement
	 */
	'path': ['id'],
	
	/*
	 * Define user input from query string
	 * Route will be /procedure/example?foo=hello&bar=world
	 / The values will be available as :foo and :bar in the PDO statement
	 */
	'query': ['foo', 'bar'],
	
	/*
	 * Define user input from the request body
	 */
	'body': ['buz'],
	
	'statement': '
		SELECT p.id, p.content, c.name 
		FROM posts p
		INNER JOIN categories c ON c.id = p.category_id
		WHERE p.id = :id AND categories.name = :foo
	',
];

return ob_get_clean();
}
}
2 changes: 2 additions & 0 deletions src/Tqdev/PhpCrudApi/Record/ErrorCode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class ErrorCode
const PAGINATION_FORBIDDEN = 1019;
const USER_ALREADY_EXIST = 1020;
const PASSWORD_TOO_SHORT = 1021;
const PROCEDURE_NOT_FOUND = 1022;

private $values = [
9999 => ["%s", ResponseFactory::INTERNAL_SERVER_ERROR],
Expand All @@ -58,6 +59,7 @@ class ErrorCode
1019 => ["Pagination forbidden", ResponseFactory::FORBIDDEN],
1020 => ["User '%s' already exists", ResponseFactory::CONFLICT],
1021 => ["Password too short (<%d characters)", ResponseFactory::UNPROCESSABLE_ENTITY],
1022 => ["Procedure '%s' not found", ResponseFactory::NOT_FOUND],
];

public function __construct(int $code)
Expand Down
11 changes: 11 additions & 0 deletions src/Tqdev/PhpCrudApi/RequestUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ public static function getOperation(ServerRequestInterface $request): string
case 'PATCH':
return 'increment';
}
case 'procedures':
Copy link
Owner

@mevdschee mevdschee Nov 28, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we merge this with the 'records' way of determining the operation? Or we implement that the operation is set to the verb in case of procedures. We could also support: /procedures/{table_name}/{verb}.sql, where {table_name} is a name you give this group of stored procedures (could be related to a table).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verb or 'records' are fine with me. If we go for 'records', how do we distinguish between read and list?

switch ($method) {
case 'POST':
return 'write';
case 'GET':
return 'read';
case 'PUT':
return 'update';
case 'DELETE':
return 'delete';
}
}
return 'unknown';
}
Expand Down
4 changes: 4 additions & 0 deletions src/procedures/example.read.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
SELECT p.id, p.content, c.name
FROM posts p
INNER JOIN categories c ON c.id = p.category_id
WHERE p.id = :id
4 changes: 4 additions & 0 deletions src/procedures/example.write.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
SELECT p.id, p.content, c.name
FROM posts p
INNER JOIN categories c ON c.id = p.category_id
WHERE p.id = <?= $id ?>
Copy link
Owner

@mevdschee mevdschee Nov 28, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a serious security issue, string concatenation in SQL with user input. Judging from your earlier comments and the different approach above, it seems that you are already aware that this is not the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, of course this is a particular bad example. But this feature (ob_start, include, ob_get_clean instead of file_get_contents) does give a lot of flexibility, for example:

INSERT INTO users SET username = :username, password = "<?= password_hash($password, PASSWORD_DEFAULT) ?>"

Or

INSERT INTO logs SET foo = :foo, user_ip = "<?= $_SERVER['REMOTE_ADDR'] ?>"