Skip to content

Commit

Permalink
Add an API spec validator to report parameter differences.
Browse files Browse the repository at this point in the history
  • Loading branch information
kohler committed Sep 12, 2024
1 parent ff77c72 commit db85fcb
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 20 deletions.
45 changes: 37 additions & 8 deletions batch/apispec.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,22 +110,51 @@ private function resolve_common_param($name) {
/** @return object */
private function expand1($fn, $method, $j) {
$x = (object) [];
$params = [];
$params = $body_properties = $body_required = [];
if ($j->paper ?? false) {
$params[] = $this->resolve_common_param("p");
}
foreach ($j->parameters ?? [] as $p) {
$optional = str_starts_with($p, "?");
$name = $optional ? substr($p, 1) : $p;
$params[] = [
"name" => $name,
"in" => "query",
"required" => !$optional
];
$required = true;
$in = "query";
for ($i = 0; $i !== strlen($p); ++$i) {
if ($p[$i] === "?") {
$required = false;
} else if ($p[$i] === "=") {
$in = "body";
} else {
break;
}
}
$name = substr($p, $i);
if ($in === "query") {
$params[] = ["name" => $name, "in" => $in, "required" => $required];
} else {
$body_properties[] = ["name" => $name];
if ($required) {
$body_required[] = $name;
}
}
}
if (!empty($params)) {
$x->parameters = $params;
}
if (!empty($body_properties)) {
$schema = (object) [
"type" => "object",
"properties" => $body_properties
];
if (!empty($body_required)) {
$schema->required = $body_required;
}
$x->requestBody = (object) [
"content" => (object) [
"application/x-www-form-urlencoded" => (object) [
"schema" => $schema
]
]
];
}
return $x;
}

Expand Down
1 change: 1 addition & 0 deletions batch/makedist.sh
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ src/api/api_search.php
src/api/api_searchconfig.php
src/api/api_session.php
src/api/api_settings.php
src/api/api_specvalidator.php
src/api/api_taganno.php
src/api/api_tags.php
src/api/api_upload.php
Expand Down
24 changes: 15 additions & 9 deletions etc/apifunctions.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@
},
{
"name": "alltags", "get": true,
"function": "AllTags_API::run",
"parameters": []
"function": "AllTags_API::run"
},
{
"name": "assign", "post": true, "redirect": true,
Expand All @@ -42,12 +41,12 @@
{
"name": "comment", "paper": true, "post": true,
"function": "Comment_API::run",
"parameters": ["c", "text", "topic", "draft", "blind", "tags", "visibility"]
"parameters": ["c", "?override", "=text", "=topic", "?=draft", "?=blind", "=tags", "=visibility"]
},
{
"name": "declinereview", "post": true, "redirect": true,
"function": "RequestReview_API::declinereview",
"parameters": ["r", "?reason"]
"parameters": ["r", "?=reason"]
},
{
"name": "decision", "paper": true, "get": true, "post": true,
Expand Down Expand Up @@ -95,8 +94,7 @@
},
{
"name": "manager", "paper": true, "get": true,
"function": "PaperPC_API::manager_api",
"parameters": []
"function": "PaperPC_API::manager_api"
},
{
"name": "manager", "paper": true, "post": true,
Expand Down Expand Up @@ -202,9 +200,14 @@
"name": "settags", "alias": "tags"
},
{
"name": "tags", "get": true, "post": true,
"name": "tags", "get": true, "paper": true,
"function": "Tags_API::run"
},
{
"name": "tags", "post": true,
"function": "Tags_API::run",
"parameters": ["?p", "?forceShow", "?=tags", "?=addtags", "?=deltags", "?=tagassignment", "?=search"]
},
{
"name": "settingdescriptions", "get": true,
"function": "Settings_API::descriptions"
Expand Down Expand Up @@ -236,6 +239,10 @@
{
"name": "tagreport", "alias": "tagmessages"
},
{
"name": "track", "post": true,
"parameters": ["track", "?tracker_start_at", "?p", "?=hotlist-info"]
},
{
"name": "trackerconfig", "post": true,
"function": "MeetingTracker::trackerconfig_api"
Expand Down Expand Up @@ -266,7 +273,6 @@
},
{
"name": "whoami", "get": true,
"function": "User_API::whoami",
"parameters": []
"function": "User_API::whoami"
}
]
5 changes: 2 additions & 3 deletions scripts/script.js
Original file line number Diff line number Diff line change
Expand Up @@ -7489,8 +7489,7 @@ function cmt_save(elt, action, really) {
if (this.files.length === 0)
this.disabled = true;
});
var arg = {p: siteinfo.paperid};
cj.cid && (arg.c = cj.cid);
var arg = {p: siteinfo.paperid, c: cj.cid || "new"};
really && (arg.override = 1);
siteinfo.want_override_conflict && (arg.forceShow = 1);
action === "delete" && (arg.delete = 1);
Expand Down Expand Up @@ -11045,7 +11044,7 @@ handle_ui.on("js-plinfo-edittags", function () {
if (focused)
focus_within(pidfe.closest("tr"));
}
$.post(hoturl("=api/tags", {p: pid, forceShow: 1}), start); // XXX should be GET
$.get(hoturl("api/tags", {p: pid, forceShow: 1}), start);
});


Expand Down
63 changes: 63 additions & 0 deletions src/api/api_specvalidator.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php
// api_specvalidator.php -- HotCRP API spec validator
// Copyright (c) 2008-2024 Eddie Kohler; see LICENSE.

class SpecValidator_API {
const F_REQUIRED = 1;
const F_BODY = 2;

static function run($uf, Qrequest $qreq) {
$known = [];
foreach ($uf->parameters ?? [] as $p) {
$flags = self::F_REQUIRED;
for ($i = 0; $i !== strlen($p); ++$i) {
if ($p[$i] === "?") {
$flags &= ~self::F_REQUIRED;
} else if ($p[$i] === "=") {
$flags |= self::F_BODY;
} else {
break;
}
}
$n = substr($p, $i);
$known[$n] = $flags;
if (($flags & self::F_REQUIRED) !== 0
&& !$qreq->has($n)) {
self::error($qreq, "required parameter `{$n}` missing");
}
}
$param = [];
foreach (array_keys($_GET) as $n) {
if (!isset($known[$n])) {
if (!in_array($n, ["post", "base", "fn", "_"])
&& ($n !== "p" || !($uf->paper ?? false))
&& !isset($known["*"])) {
self::error($qreq, "query param `{$n}` unknown");
}
} else if (($known[$n] & self::F_BODY) !== 0) {
self::error($qreq, "query param `{$n}` should be in body");
}
}
foreach (array_keys($_POST) as $n) {
if (!isset($known[$n])) {
if (!isset($known["*"])) {
self::error($qreq, "post param `{$n}` unknown");
}
} else if (!isset($_GET[$n])
&& ($known[$n] & self::F_BODY) === 0) {
self::error($qreq, "post param `{$n}` should be in query");
}
}
}

static function error(Qrequest $qreq, $error) {
$nav = $qreq->navigation();
$url = substr($nav->self(), 0, 100);
$out = $qreq->conf()->opt("validateApiSpec");
if (is_string($out)) {
@file_put_contents($out, "{$url}: {$error}\n", FILE_APPEND);
} else {
error_log("{$url}: {$error}");
}
}
}
3 changes: 3 additions & 0 deletions src/pages/p_api.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ static private function normal_api($fn, $user, $qreq) {
JsonCompletion::$allow_short_circuit = true;
$conf = $user->conf;
$uf = $conf->api($fn, $user, $qreq->method());
if ($conf->opt("validateApiSpec")) {
SpecValidator_API::run($uf, $qreq);
}
$jr = $conf->call_api_on($uf, $fn, $user, $qreq, $conf->paper);
if ($uf
&& ($uf->redirect ?? false)
Expand Down

0 comments on commit db85fcb

Please sign in to comment.