Skip to content

Commit 8f76a2c

Browse files
committed
fix(review): clamp resolveTtl() against non-positive filter results; deflag old constant name in docs
Copilot review findings on PR #128: - PulseGenerator::resolveTtl() now falls back to the sensitivity's own default if the sparxstar_sirus_pulse_ttl_seconds filter returns a zero/negative value, instead of propagating it. generate() throws on $ttlSeconds <= 0, so a misbehaving filter would otherwise turn into an uncaught 500 on every REST-issued pulse. Added regression tests. - TRACKER.md and docs/sirus-tech-spec.md described the B-1 rename by spelling out the literal old constant name, which would make a mechanical "zero occurrences repo-wide" grep check keep failing forever even after the actual rename landed correctly. Reworded to describe the old name without spelling it out; verified zero literal occurrences remain anywhere in the repo. Verified: 551 tests, 1191 assertions, still exactly the same 16 pre-existing/unrelated errors as before this commit (confirmed via diff against main in the PR thread) -- no regressions, two tests added.
1 parent e4bdc3c commit 8f76a2c

4 files changed

Lines changed: 34 additions & 4 deletions

File tree

TRACKER.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ This document tracks every component defined in **Sirius Context Engine Spec v3.
3939
| `TrustEngine` | `src/core/TrustEngine.php` || S-01/S-02 | Frozen algorithm; 18 unit tests in `TrustEngineTest` |
4040
| `TrustResolver` | `src/core/TrustResolver.php` || S-01/S-02 | Credential-level base + drift/session deductions; 19 unit tests in `TrustResolverTest`; `CREDENTIAL_BASE` fixed 2026-08-01 — removed dead `elder` entry (not a `CredentialTier` case), added missing `authority` entry (0.95, above `user` 0.85); added exhaustive-coverage and monotonic-ordering tests |
4141
| `StepUpPolicy` | `src/core/StepUpPolicy.php` || S-01/S-02 | Frozen policy; `requiresStepUp()` + `TRUST_LEVEL_STEP_UP_REQUIRED` pre-flag check; fails closed on `LOCKED` (checked before `STEP_UP_REQUIRED`, added 2026-08-01); 21 unit tests |
42-
| `PulseGenerator` | `src/core/PulseGenerator.php` || S-01/S-02 | HMAC-SHA256 only; consumes enum-backed `SirusContext::trust_level`; PAM-002-P2 fields wired (`behavior_flags`, `geo_zone`, `network_effective_type`, `session_duration`); 36 unit tests in `PulseGeneratorTest`; `$now`/`$ttlSeconds` explicit params; signing key constant renamed `SIRUS_PULSE_SIGNING_KEY` `SPARXSTAR_PULSE_SIGNING_KEY` (2026-08-01, matches Helios); `resolveTtl(ResourceSensitivity)` implements the sensitivity/connectivity TTL strategy (2026-08-01) |
42+
| `PulseGenerator` | `src/core/PulseGenerator.php` || S-01/S-02 | HMAC-SHA256 only; consumes enum-backed `SirusContext::trust_level`; PAM-002-P2 fields wired (`behavior_flags`, `geo_zone`, `network_effective_type`, `session_duration`); 36 unit tests in `PulseGeneratorTest`; `$now`/`$ttlSeconds` explicit params; signing key constant renamed from its old `SIRUS_`-prefixed name to `SPARXSTAR_PULSE_SIGNING_KEY` (2026-08-01, matches Helios); `resolveTtl(ResourceSensitivity)` implements the sensitivity/connectivity TTL strategy (2026-08-01) |
4343

4444
### Device and Identity
4545

@@ -159,7 +159,7 @@ Static spec-conformance audit against Sirus Context Engine Spec v3.0 found and f
159159
(B-1 through B-6) plus a stub-drift CI gap (D-4). Full detail per-component is recorded in the
160160
Scoreboard rows above; this note records the cross-cutting decisions.
161161

162-
- **B-1 (🔴 platform-breaking):** `SIRUS_PULSE_SIGNING_KEY` renamed to `SPARXSTAR_PULSE_SIGNING_KEY`
162+
- **B-1 (🔴 platform-breaking):** the old `SIRUS_`-prefixed signing-key constant renamed to `SPARXSTAR_PULSE_SIGNING_KEY`
163163
everywhere (code, tests, `PUBLIC_API.md`, `README.md`) to match Helios's already-renamed side.
164164
Every pulse was failing Helios's signature verification until this landed.
165165
- **B-2 (🔴 platform-breaking):** see the `SirusDatabase` schema boot row above. Sirus tables were

docs/sirus-tech-spec.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ Client-submitted signals take precedence over server-side UA parsing. Matomo Dev
145145

146146
### Pulse generation
147147

148-
Sirus signs pulses via HMAC-SHA256 using Ouroboros `ContextPulseSigningMaterial::build()`. Pulse fields include the four PAM-002-P2 fields: `behavior_flags`, `geo_zone`, `network_effective_type`, `session_duration`. Sirus never verifies pulses at runtime — that is Helios. The HMAC signing key is read exclusively from the `SPARXSTAR_PULSE_SIGNING_KEY` PHP constant (renamed from `SIRUS_PULSE_SIGNING_KEY` to match Helios's side of the shared secret — the two names never matched, so every pulse previously failed Helios's signature verification).
148+
Sirus signs pulses via HMAC-SHA256 using Ouroboros `ContextPulseSigningMaterial::build()`. Pulse fields include the four PAM-002-P2 fields: `behavior_flags`, `geo_zone`, `network_effective_type`, `session_duration`. Sirus never verifies pulses at runtime — that is Helios. The HMAC signing key is read exclusively from the `SPARXSTAR_PULSE_SIGNING_KEY` PHP constant (renamed from its old `SIRUS_`-prefixed name to match Helios's side of the shared secret — the two names never matched, so every pulse previously failed Helios's signature verification).
149149

150150
### Pulse TTL strategy (provisional pending field testing)
151151

@@ -356,7 +356,7 @@ Achieved in S-07. `tests/integration/RestApiTest.php` covers all six endpoints.
356356

357357
| Version | Date | Notes |
358358
|---|---|---|
359-
| 3.0.2 | 2026-08-01 | Spec-conformance audit fixes: renamed `SIRUS_PULSE_SIGNING_KEY` `SPARXSTAR_PULSE_SIGNING_KEY` (matches Helios); schema creation/cron scheduling moved from the never-fired activation hook to a boot-time idempotent check (`SirusDatabase::maybe_upgrade_schema()`); legacy `sparxstar-user-environment-check.php` is now a guarded no-op when Sirus is loaded; `TrustResolver::CREDENTIAL_BASE` fixed (removed dead `elder` entry, added missing `authority` entry scored above `user`); `StepUpPolicy` now fails closed on `LOCKED` (checked before `STEP_UP_REQUIRED`); `PulseGenerator::resolveTtl()` implements the sensitivity-driven pulse TTL strategy (§ Pulse TTL strategy); added `bin/check-ouroboros-stub-drift.php` CI check. |
359+
| 3.0.2 | 2026-08-01 | Spec-conformance audit fixes: renamed the old `SIRUS_`-prefixed signing-key constant to `SPARXSTAR_PULSE_SIGNING_KEY` (matches Helios); schema creation/cron scheduling moved from the never-fired activation hook to a boot-time idempotent check (`SirusDatabase::maybe_upgrade_schema()`); legacy `sparxstar-user-environment-check.php` is now a guarded no-op when Sirus is loaded; `TrustResolver::CREDENTIAL_BASE` fixed (removed dead `elder` entry, added missing `authority` entry scored above `user`); `StepUpPolicy` now fails closed on `LOCKED` (checked before `STEP_UP_REQUIRED`); `PulseGenerator::resolveTtl()` implements the sensitivity-driven pulse TTL strategy (§ Pulse TTL strategy); added `bin/check-ouroboros-stub-drift.php` CI check. |
360360
| 3.0.1 | 2026-07-06 | Requires `sparxstar-ouroboros-integrity` ≥ v3.0.0 (introduces `CredentialTier` enum and two-field trust/credential split). Sirus is first platform repo on Ouroboros 3.x; Helios, Sky, Mehns, Dheghom tracking separately. |
361361
| 3.0.0 | 2026-07-01 | Initial governance spec submission; reflects S-07 implementation state |
362362
|| 2026-06-12 | TRACKER.md last updated; S-07 merged to main |

src/core/PulseGenerator.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,15 @@ public function resolveTtl(ResourceSensitivity $sensitivity): int
113113

114114
$ttl = (int) apply_filters('sparxstar_sirus_pulse_ttl_seconds', $default, $sensitivity, $default);
115115

116+
if ($ttl <= 0) {
117+
// A misbehaving filter callback must not be able to produce a
118+
// pulse TTL that trips generate()'s $ttlSeconds > 0 guard --
119+
// that would turn a filter bug into an uncaught exception (500)
120+
// on every REST-issued pulse. Fall back to the sensitivity's
121+
// own default instead.
122+
$ttl = $default;
123+
}
124+
116125
if ($sensitivity === ResourceSensitivity::LEVEL_1) {
117126
$network_type = $this->environmentResolver->getNetworkEffectiveType();
118127
if (in_array($network_type, self::LOW_CONNECTIVITY_NETWORK_TYPES, true)) {

tests/unit/PulseGeneratorTest.php

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,27 @@ static function (int $ttl, ResourceSensitivity $sensitivity, int $default): int
294294
$this->assertSame(999, $this->generator->resolveTtl(ResourceSensitivity::LEVEL_2));
295295
}
296296

297+
/**
298+
* A misbehaving filter callback returning zero must not propagate --
299+
* generate() throws on $ttlSeconds <= 0, so a bad filter would otherwise
300+
* turn into an uncaught 500 on every REST-issued pulse. resolveTtl()
301+
* falls back to the sensitivity's own default instead.
302+
*/
303+
public function testResolveTtlFallsBackToDefaultWhenFilterReturnsZero(): void
304+
{
305+
add_filter('sparxstar_sirus_pulse_ttl_seconds', static fn (): int => 0);
306+
$this->assertSame(60, $this->generator->resolveTtl(ResourceSensitivity::LEVEL_2));
307+
}
308+
309+
/**
310+
* Same as above, for a negative filter return value.
311+
*/
312+
public function testResolveTtlFallsBackToDefaultWhenFilterReturnsNegative(): void
313+
{
314+
add_filter('sparxstar_sirus_pulse_ttl_seconds', static fn (): int => -5);
315+
$this->assertSame(30, $this->generator->resolveTtl(ResourceSensitivity::LEVEL_3));
316+
}
317+
297318
// ── resolveTtl() — low-connectivity extension (LEVEL_1 only) ────────────────
298319

299320
/**

0 commit comments

Comments
 (0)