Merge lp:~marcustomlinson/unity-scopes-api/strict_idle_timeout into lp:unity-scopes-api

[Marcus Tomlinson]

Be much stricter on idle timeout (1s to 5min)

Be much stricter on idle timeout (1s to 5min). Not only are scopes currently allowed to set very very large timeouts, we seem to allow disabling of the timeout entirely (a value of -1)

[Michi Henning (michihenning)]

Looks good to me. Did we have problems with a scopes setting unreasonable values?

Not sure where the -1 problem is. The previous code complained if the value was < 0, meaning that you can have a scope that times out immediately. (Not that useful, I admit.)

I can't recall why we allowed maxint :-(

review: Approve

[Marcus Tomlinson (marcustomlinson)]

> Looks good to me. Did we have problems with a scopes setting unreasonable
> values?

I don't know of a particular case where a scope is doing this, but that's part of the concern I suppose. Feels like a bit of a security risk allowing scopes to essentially act like daemons this way.

>
> Not sure where the -1 problem is. The previous code complained if the value
> was < 0, meaning that you can have a scope that times out immediately. (Not
> that useful, I admit.)

If you look at the diff we allowed ((>=0 && <= max) || -1)

>
> I can't recall why we allowed maxint :-(

Me neither.

[Michi Henning (michihenning)]

> I don't know of a particular case where a scope is doing this, but that's part
> of the concern I suppose. Feels like a bit of a security risk allowing scopes
> to essentially act like daemons this way.

Back then, we knew little about how scopes would be used and what they might be doing. I suspect that's why it ended up being so liberal.

> If you look at the diff we allowed ((>=0 && <= max) || -1)

Ah, yes. I didn't read it closely enough. Anyway, I think it's fine a limit it to five minutes. If that breaks some scope, we'll find out soon enough.

[lp:~marcustomlinson/unity-scopes-api/strict_idle_timeout]

691. By Marcus Tomlinson on 2016-10-13

Oops, fix ScopeConfig_test.cpp

Unmerged revisions

=== modified file 'doc/tutorial.dox'
--- doc/tutorial.dox	2016-06-15 09:16:47 +0000
+++ doc/tutorial.dox	2016-10-12 12:46:20 +0000
@@ -1560,7 +1560,8 @@
 
 The `IdleTimeout` key controls how long a scope can remain idle before it is told to stop by the registry (or
 killed if it does not stop within 4 seconds). The default idle timeout is 40 seconds, meaning that a
-scope will be told to stop if no query was sent to it for that amount of time.
+scope will be told to stop if no query was sent to it for that amount of time. Only values in the range 1 to
+300 seconds are accepted.
 
 `ResultTtl` determines how long results should be cached by the UI before they are considered "stale"
 and should be refreshed. `None` indicates that results remain valid indefinitely; `Small` indicates

=== modified file 'src/scopes/internal/ScopeConfig.cpp'
--- src/scopes/internal/ScopeConfig.cpp	2016-03-31 09:27:33 +0000
+++ src/scopes/internal/ScopeConfig.cpp	2016-10-12 12:46:20 +0000
@@ -173,12 +173,11 @@
 
     idle_timeout_ = get_optional_int(scope_config_group, idle_timeout_key, DFLT_SCOPE_IDLE_TIMEOUT);
 
-    // Negative values and values greater than max int (once multiplied by 1000 (s to ms)) are illegal
-    const int max_idle_timeout = std::numeric_limits<int>::max() / 1000;
-    if ((idle_timeout_ < 0 || idle_timeout_ > max_idle_timeout) && idle_timeout_ != -1)
+    // Values less than 1s and greater than 300s are illegal
+    if (idle_timeout_ < 1 || idle_timeout_ > 300)
     {
         throw_ex("Illegal value (" + std::to_string(idle_timeout_) + ") for " + idle_timeout_key +
-                 ": value must be >= 0 and <= " + std::to_string(max_idle_timeout));
+                 ": value must be >= 1 and <= 300");
     }
 
     results_ttl_type_ = ScopeMetadata::ResultsTtlType::None;