Skip to content

fix(gate-50): the app-id widening reached settings read-outs, which have nothing to guard - #282

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-50-appid-scope
Aug 9, 2026
Merged

fix(gate-50): the app-id widening reached settings read-outs, which have nothing to guard#282
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-50-appid-scope

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

fix(gate-50): the app-id widening reached settings read-outs, which have nothing to guard

Follow-up to #280, caught by re-running the 12-repo before/after sweep and
comparing FINDING COUNTS rather than verdicts. The verdict-level diff I ran
first showed 26 changes and missed this entirely, because the verdict did not
move: softwarecatalog was FAIL before and FAIL after — at 23 findings and then
at 64.

What went wrong

#280 fixed a real blind spot: gate-50's app-id argument had to be a QUOTED
STRING, so every read written the fleet-standard way — getValueString( Application::APP_ID, 'listing_register', '') — was invisible. That was
measured at 7 security-relevant reads across 5 repos.

The regex I wrote to fix it accepted [^,()]+, i.e. anything up to the comma.
That is a strictly larger widening than the defect measured, and it takes
$app and $this->appName as well. 47 of softwarecatalog's 41 new findings
are entries in an array literal that assembles the admin settings payload:

'sendgridApiKey' => $this->config->getValueString($app, 'email_sendgrid_api_key', ''),

No defense is being deactivated in a settings read-out, and there is nothing
to guard — you cannot add an empty-check to an entry in an array literal. The
finding has no legitimate end state, which is the unclosable-gate shape (#252)
and the thing most likely to make people stop reading this gate.

What this changes

The accepted app-id shapes are now exactly the ones measured as blind: a
quoted literal, or a class constant (Application::APP_ID, self::APP_ID,
static::APP_ID). Measured after:

softwarecatalog   23 -> 17   (0 added; the 6 removed are #280's window fix,
                              verified by hand: ArchiMateService.php:1804-1807
                              and ArchiMateImportService.php:2050-2052 are all
                              guarded by `if ($rawRegisterId !== null &&
                              $rawRegisterId !== '' && ...)` or by a same-line
                              `if (getValueString(...) !== '')`)
procest            3 ->  0   (all three were the multi-line-window false
                              positives #280 fixed)
larpingapp/decidesk/opencatalogi/openconnector/doriath/nldesign — unchanged

So against the package before #280, this band now reports strictly fewer
gate-50 findings and every removal is a verified false positive, while the 7
constant-app-id reads that were invisible are caught.

The blind spot is now stated, not silent

A read whose app id is a plain VARIABLE remains invisible to this gate. That
is not a safe shape — it is an unmeasured one, and separating the settings
read-outs from the real scope decisions among them needs a data-flow question
a regex cannot ask. Two arms pin the boundary rather than an ideal:

C9 settings read-outs with $app / $this->appName -> PASS, with the
number and the reason in the test's own comment
C10 the SAME read written with a class constant -> FAIL, so C9 is a
boundary and not a hole the gate fell through

Note on method

Comparing verdicts across a sweep is not enough: FAIL -> FAIL hid a 23 -> 64
change. Count, not verdict, is the comparison that would have caught this
before the merge.

…ave nothing to guard

Follow-up to #280, caught by re-running the 12-repo before/after sweep and
comparing FINDING COUNTS rather than verdicts. The verdict-level diff I ran
first showed 26 changes and missed this entirely, because the verdict did not
move: softwarecatalog was FAIL before and FAIL after — at 23 findings and then
at 64.

## What went wrong

#280 fixed a real blind spot: gate-50's app-id argument had to be a QUOTED
STRING, so every read written the fleet-standard way — `getValueString(
Application::APP_ID, 'listing_register', '')` — was invisible. That was
measured at 7 security-relevant reads across 5 repos.

The regex I wrote to fix it accepted `[^,()]+`, i.e. anything up to the comma.
That is a strictly larger widening than the defect measured, and it takes
`$app` and `$this->appName` as well. 47 of softwarecatalog's 41 new findings
are entries in an array literal that assembles the admin settings payload:

    'sendgridApiKey' => $this->config->getValueString($app, 'email_sendgrid_api_key', ''),

No defense is being deactivated in a settings read-out, and there is nothing
to guard — you cannot add an empty-check to an entry in an array literal. The
finding has no legitimate end state, which is the unclosable-gate shape (#252)
and the thing most likely to make people stop reading this gate.

## What this changes

The accepted app-id shapes are now exactly the ones measured as blind: a
quoted literal, or a class constant (`Application::APP_ID`, `self::APP_ID`,
`static::APP_ID`). Measured after:

    softwarecatalog   23 -> 17   (0 added; the 6 removed are #280's window fix,
                                  verified by hand: ArchiMateService.php:1804-1807
                                  and ArchiMateImportService.php:2050-2052 are all
                                  guarded by `if ($rawRegisterId !== null &&
                                  $rawRegisterId !== '' && ...)` or by a same-line
                                  `if (getValueString(...) !== '')`)
    procest            3 ->  0   (all three were the multi-line-window false
                                  positives #280 fixed)
    larpingapp/decidesk/opencatalogi/openconnector/doriath/nldesign — unchanged

So against the package before #280, this band now reports strictly fewer
gate-50 findings and every removal is a verified false positive, while the 7
constant-app-id reads that were invisible are caught.

## The blind spot is now stated, not silent

A read whose app id is a plain VARIABLE remains invisible to this gate. That
is not a safe shape — it is an unmeasured one, and separating the settings
read-outs from the real scope decisions among them needs a data-flow question
a regex cannot ask. Two arms pin the boundary rather than an ideal:

  C9   settings read-outs with `$app` / `$this->appName` -> PASS, with the
       number and the reason in the test's own comment
  C10  the SAME read written with a class constant -> FAIL, so C9 is a
       boundary and not a hole the gate fell through

## Note on method

Comparing verdicts across a sweep is not enough: FAIL -> FAIL hid a 23 -> 64
change. Count, not verdict, is the comparison that would have caught this
before the merge.
@rubenvdlinde
rubenvdlinde merged commit 48c88ba into main Aug 9, 2026
31 checks passed
rubenvdlinde added a commit that referenced this pull request Aug 9, 2026
The branch was rebased from cdfbd7a onto 48c88ba after four gate-package
releases landed mid-session (#272, #275, #276, #280/#282). The rebase gave
the same content a new history, which is a force-push, and force-push is
blocked on shared branches for good reason. The tree here is IDENTICAL to
the rebased HEAD — this commit only re-attaches the old tip as a second
parent so the push is a fast-forward.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant