Possible bug in API operation setArticleLabel

Tiny Tiny RSS Development
1

The documentation states that the ‘assign’ parameter is a boolean, but the code (on line 469 of classes/api.php) only accepts “true”:

$assign = (bool) ($this->dbh->escape_string($_REQUEST['assign']) == "true");

The following line might be better:

$assign = filter_var($this->dbh->escape_string($_REQUEST['assign']), FILTER_VALIDATE_BOOLEAN);
2

In PHP pretty much every request parameter is a string, unless you want to serialize the data which is just a waste of time on both sides. The closest you’ll get to boolean is a string 0 or 1.

That being said, the API code is correct in how it’s done because everything is coming in as a string. So you’d send the string “true” or “false” (anything not “true”, really). The PHP code evaluates that into a boolean.

If anything the wiki documentation should be clarified.

e: Your proposal is not wrong, I’m only saying the code as-is does work within the understanding of how PHP receives its data. Also, I’m betting that particular line of code was written before the filter_var function came into being.

3

That’s a well reasoned argument, but I can’t agree, because:

  1. We’re not talking traditional application/x-www-form-urlencoded input, here; the input is explicitly defined as JSON, and JSON may encode actual booleans
  2. It’s a regression: before 2017-04-27 straight JSON booleans worked fine; only "false" gave an unexpected result

Especially given the latter, I really do think the comparison should be adjusted. I know of at least one client which is sending true rather than "true", and there are probably others.

As an aside, you’d lose that bet about filter_var(). :wink: The function was introduced in PHP 5.2.0 (2006-11-02), and the setArticleLabel operation was added 2011-12-17, over five years later. Still, I wouldn’t blame anyone for not knowing about it even today: PHP is absolutely full of obscure functions that do the strangest of things (not to mention that there was resistance to PHP 5 for a ridiculously long time). I continually find new ones I’d somehow missed in previous perusals.

4

I was not about to review the commit history on my phone to find when it was introduced. It just wasn’t that important to me.

Fair enough on your other points though. Looking at the specific commit it looks like it was made around the time some tests were put into place; the difference is subtle in how the data is handled but different nonetheless.

You’re welcome to submit a patch and see what fox does with it.

5

i appreciate the PR and stuff but i think in this particular case it would be better to document on the API wiki page that expected syntax for boolean is string literal “true” for true and anything else for false and leave it at that.

i’m aware of filter_var() but adding it to this particular case only just feels wrong to me for whatever 'tism reason.

6

The only problem with changing the documentation is that other API operations don’t act in the same way. I was already aware that getCategories accepts proper booleans, and a quick perusal of the code reveals that getHeadlines, getFeeds, getFeedTree, and getArticle all accept literal booleans; they do so using the sql_bool_to_bool helper function, it seems.

Here’s a proper patch:

 classes/api.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/classes/api.php b/classes/api.php
index 97050e10..bb4d3324 100644
--- a/classes/api.php
+++ b/classes/api.php
@@ -466,7 +466,7 @@ class API extends Handler {
 
 		$article_ids = array_filter(explode(",", $this->dbh->escape_string($_REQUEST["article_ids"])), is_numeric);
 		$label_id = (int) $this->dbh->escape_string($_REQUEST['label_id']);
-		$assign = (bool) ($this->dbh->escape_string($_REQUEST['assign']) == "true");
+		$assign = sql_bool_to_bool($_REQUEST['assign']);
 
 		$label = $this->dbh->escape_string(Labels::find_caption(
 			Labels::feed_to_label_id($label_id), $_SESSION["uid"]));
7

you should be able to use both JSON string “true” and JSON boolean true, it shouldn’t make any difference because for php it’s all pretty much same thing anyway and setArticleLable() doesn’t use php bespoke “===” operator when comparing with “true”.

if those are using sql_bool_to_bool() you can also pass string “t” i guess which is a postgresql-specific thing but that doesn’t really change anything (you shouldn’t be using this variant anyway for the API call).

  •  $assign = sql_bool_to_bool($_REQUEST['assign']);

this change accomplishes literally nothing as far as using the API is concerned. why make it then?

e: are you sure you understand how php types work? run something like this

if (true == "true") echo "well shit\n";
8

Yes, I agree, it shouldn’t make any difference, but it does (without the patch), because $this->dbh->escape_string(true) == "true" is false: the escape function returns "1", and "1" == "true" is false. Hence the bug and hence the need for a patch.

9

wait what. why would be “true” escaped to “1”? what the fuck

10

Because (string) true is "1".

11

ah idd, i guess you’re right. i suppose in this case just running sql_bool_to_bool() would be the easiest way out.

e: done

e2: tbh i’m not sure why this was written in this way, bool and escape and shit. maybe i was drunk or something.