Skip to content

Commit 69ec5ea

Browse files
committed
docs(observability-map): disclose the free point auth-scope carries, and correct the stale figures
Five disclosure and accuracy items from the whole-branch review, no behaviour change. auth-scope is applicable only when a route uses a builder, and auth-boundary passes any route that uses a builder, so auth-scope applicable structurally implies auth-boundary pass: all 19 carry the same detail string. That free point is a third to a quarter of each of their scores. The 19 average 59.7 as scored and 44.6 without auth-boundary, and settings.team, a confirmed cross-org exposure, scores 25 rather than 0 because of it. Said in the README where the check is explained, since the finding is the thing to read there, not the score. Then the stale numbers. Eight docstrings still quoted the pre-round-C baseline of 15, including the code-side disclosure of the largest parked residual, where the README had it right and the code understated it. Each is re-derived where it can be, from the live corpus run, and dated where it cannot: two of them measure rejected variants that would have to be reimplemented to re-measure, so they now say which tree they were taken against. One of them read "takes the tree back to 15 to 42", which was garbled as well as stale. terminal.ts said the audit finding would repeat 52 times where it is 46. The round A forging residual was in the ledger only and is now a Known limits bullet beside try { String(0); }, with both figures live. The workflow said the corpus takes roughly three minutes where it is about four and a half for 44 entries. And score.ts now says outright that it answers the delegating case for every check, because two checks tested ep.delegating themselves and both branches were unreachable behind that short-circuit.
1 parent 94f5184 commit 69ec5ea

5 files changed

Lines changed: 31 additions & 12 deletions

File tree

.github/workflows/observability-map.yml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@ permissions:
1616

1717
jobs:
1818
# The tree-scale mutation corpus: every known laundering shape applied to the whole route tree,
19-
# asserting the score does not rise. Roughly three minutes, which is why it is gated out of the
20-
# package's default `pnpm test` and run here instead. Unlike the report job below it has no token
21-
# to lose, so it runs for fork PRs too, and unlike the report job it is allowed to fail the build.
19+
# asserting the score does not rise. Roughly four and a half minutes for 44 entries, which is why
20+
# it is gated out of the package's default `pnpm test` and run here instead. Unlike the report
21+
# job below it has no token to lose, so it runs for fork PRs too, and unlike the report job it is
22+
# allowed to fail the build.
2223
mutation-corpus:
2324
name: 🧬 Mutation corpus
2425
runs-on: warp-ubuntu-latest-x64-4x

internal-packages/observability-map/README.md

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ every catch clause in the tree used to score it 100, which meant the metric paid
5656
error handling.
5757

5858
The property behind that is now a test corpus rather than a claim. `src/mutationCorpus.test.ts`
59-
applies 42 semantics-preserving or handling-deleting rewrites to the whole route tree in a temp copy
59+
applies 44 semantics-preserving or handling-deleting rewrites to the whole route tree in a temp copy
6060
and asserts three things for each: the published global does not rise, the mean over the routes
6161
measured in both runs does not rise, and for a semantics-preserving rewrite no individual route's
6262
score rises or drops out of the measured set. Every laundering shape a reviewer has found on this
@@ -78,7 +78,7 @@ One of those is still open and the corpus says so. A catch over `try { 0; }` is
7878
from 19 to 44 and raises 224 routes. Telling an inert call from one that can throw needs types the
7979
scanner does not have.
8080

81-
The honest statement is "these 41 rewrites are defended, here they are, and here is the one that is
81+
The honest statement is "these 43 rewrites are defended, here they are, and here is the one that is
8282
not", not "unpaddable". One entry, `dead-classifying-try-with-call`, runs as an expected failure
8383
with the residual written out beside it. The corpus takes about four and a half minutes, so it is
8484
gated behind `OBS_MAP_MUTATION_CORPUS=1` and run as its own CI job rather than in `pnpm test`. If
@@ -108,8 +108,9 @@ rather than celebrating.
108108
builder-wrapped route also narrows itself to the caller, in every export, by declaring
109109
`authorization` or by filtering on the caller's own id.
110110
- **request-context**: when this entry point's failure is reported, is the tenant named.
111-
- **audit-trail**: does a sensitive mutation leave a record of who did it. Nothing in the webapp
112-
writes one, so every applicable entry point fails.
111+
- **audit-trail**: does a sensitive mutation leave a record of who did it. Three routes do, all of
112+
them impersonation paths reaching `prisma.impersonationAuditLog.create` in
113+
`models/admin.server.ts`; the other 46 do not.
113114

114115
`audit-trail` is excluded from the score. The other four are in it.
115116

@@ -124,7 +125,7 @@ CHECKS
124125
auth-boundary 62 applicable, 59 pass, 0 sole, global without it 15
125126
auth-scope 19 applicable, 17 pass, 0 sole, global without it 18
126127
request-context 412 applicable, 11 pass, 223 sole, global without it 65
127-
audit-trail 49 applicable, 0 pass, 0 sole, not in the score
128+
audit-trail 49 applicable, 3 pass, 0 sole, not in the score
128129
```
129130

130131
`sole` is the figure that says the most: 223 of the 412 measured entry points have exactly one
@@ -140,7 +141,7 @@ is made of`).
140141

141142
## Two findings are headlines, not list entries
142143

143-
`audit-trail` fails 49 of 49, and `request-context` fails 401 of 412. Printing either one per route
144+
`audit-trail` fails 46 of 49, and `request-context` fails 401 of 412. Printing either one per route
144145
would bury the route-specific findings under the same sentence repeated hundreds of times, so both
145146
are reported as a figure: the `AUDIT` and `CONTEXT` lines. 328 entry points fail nothing except
146147
`request-context` and appear only in that figure, which leaves 76 in the fix list. An entry that
@@ -270,6 +271,15 @@ put `members: { some: { userId } }` on the lookup.
270271
That per-export rule is the load-bearing half. Both of those files scope themselves in their OTHER
271272
export, so an entry-point-wide reading passed them, and the exposure is per export.
272273

274+
One thing to know before reading a score on any of these 19 routes. `auth-scope` is only applicable
275+
when the route uses a builder, and `auth-boundary` passes any route that uses a builder, so
276+
**`auth-scope` applicable structurally implies `auth-boundary` pass**: all 19 carry the same
277+
`auth-boundary` detail, "authenticated by the builder". That free point is a third or a quarter of
278+
each of their scores. The 19 average 59.7 as scored and 44.6 with `auth-boundary` taken out, and
279+
`settings.team`, a confirmed cross-org exposure, scores 25 rather than 0 because of it. The score is
280+
not wrong, since the builder does authenticate. It is just less informative here than it looks, and
281+
the finding is the thing to read.
282+
273283
## Suppression
274284

275285
```ts
@@ -371,7 +381,12 @@ Read these before trusting a specific verdict.
371381
`export { action } from "./x"` beside a loader written in the file is not counted as delegating,
372382
so half the route is scored and half is invisible.
373383
- **`try { String(0); }` still buys a pass.** The open corpus entry, above. It is the largest single
374-
hole known in the tool.
384+
hole known in the tool: measured live, it takes the tree from 19 to 44 and raises 224 routes.
385+
- **A forged tenant field buys a pass too.** The gaming boundary above, restated here because it
386+
belongs on this list: `request-context` reads the field name, never the value, so a codemod
387+
writing `environmentId: "obs-map"` into every in-catch log call takes the global from 19 to 29.
388+
Unlike the entry above this one is not a bug to fix, since no syntactic check can tell a real
389+
tenant id from a constant, but it bounds what the number can mean either way.
375390
- **The score is a mean of means over a heuristic.** Read the fix list, the two headline figures and
376391
the CHECKS block. Watching the single number for small movements will mislead you.
377392

internal-packages/observability-map/src/checks/errorClassification.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ export function usesBuilder(ep: EntryPoint): boolean {
154154
*
155155
* A clause whose try block holds nothing that could raise is read as no clause at all,
156156
* `guardCanRaise` on the evidence. Prepending `try { 0; } catch (e) { if (e instanceof Error) {
157-
* return json(x, { status: 400 }); } throw e; }` to every body took the tree from 15 to 42 and
157+
* return json(x, { status: 400 }); } throw e; }` to every body takes the tree from 19 to 44 and
158158
* raised 224 routes, because the 261 routes that catch nothing were sitting at not-applicable and a
159159
* dead clause moved each of them to pass.
160160
*

internal-packages/observability-map/src/report/terminal.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ const gauge = (score: number | null) => {
1313
/**
1414
* Failing checks that actually feed `score`. `audit-trail` is deliberately excluded here: it is
1515
* excluded from the score for the same reason (see `score.ts`), and every sensitive mutation fails
16-
* it today, so folding it in would flood this list with the same finding repeated 52 times instead
16+
* it today, so folding it in would flood this list with the same finding repeated 46 times instead
1717
* of the fixable, route-specific gaps the list exists to surface. That gap is reported once, as
1818
* `AUDIT`, below.
1919
*/

internal-packages/observability-map/src/score.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,9 @@ export type MapReport = {
112112
* the file holds no handler function and no builder call, so there is nothing for a check to read
113113
* and no check may claim a verdict. `request-context` would otherwise fail such a route for leaving
114114
* its failures to the central handler, an accusation about a body this file does not contain.
115+
*
116+
* Because it is answered here, no check tests `ep.delegating` itself. Two did, and both branches
117+
* were unreachable.
115118
*/
116119
const DELEGATED_CHECKS = (): CheckResult[] =>
117120
CHECKS.map((c) => ({

0 commit comments

Comments
 (0)