Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ operations, operational tooling, and OpenID Connect.
### Fixed — the published container images

Four defects found by publishing 0.8.0 and then *running* what was published.
All 18 CI jobs passed over every one of them, because the `docker` job builds
images and does not exercise what it built.
All 18 CI jobs passed over every one of them — not because CI ignores images,
but because it only covers one. The `docker` job builds the API image, starts
it, probes its shutdown behaviour and fails if it runs as root. `Dockerfile.tools`
and `pangolin_ui/Dockerfile` are never built in CI at all, so the two images
with defects were the two nothing tested.

- **`Dockerfile.tools` still pinned `rust:1.88`.** A-36 raised the API image to
match the workspace MSRV and missed the CLI image, which then failed to
Expand Down
234 changes: 234 additions & 0 deletions ROADMAP_0.9.0.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
# Roadmap — 0.9.0

0.8.0 was about making the server correct and safe. 0.9.0 is about the two
things 0.8.0 exposed but did not finish: **authorization that is verified rather
than assumed**, and **a release process whose output is what the tag says it
is**.

Every item below was confirmed against the code on `main` at the time of
writing, with the command that confirms it. Nothing here is recalled from
memory — this project's recurring failure is things that look done and are not,
so an item that cannot be demonstrated does not belong on the list.

Ordered by what would hurt most if left undone.

---

## Bucket 1 — Authorization correctness

### A1. `TenantAdmin` bypasses the scope check entirely

`pangolin_api/src/authz.rs:39`

```rust
if session.role == RoleEnum::TenantAdmin {
// TODO: Check if scope matches session.tenant_id
return Ok(true);
}
```

Any `TenantAdmin` gets `true` for **any** scope, and `PermissionScope` carries no
tenant: `Catalog { catalog_id }`, `Namespace { catalog_id, namespace }`,
`Asset { catalog_id, namespace, asset_id }`. The function therefore *cannot*
verify the target belongs to the caller's tenant without a lookup it never does.

Whether this is reachable depends on all 41 call sites independently enforcing
tenant scoping first. **That has not been verified, and this is the single most
important thing in 0.9.0.** In a multi-tenant catalog, a cross-tenant read is
the worst outcome the product has.

- Audit all 41 `check_permission(` call sites for prior tenant enforcement.
- Resolve the scope's owning tenant inside `check_permission` and compare it to
`session.tenant_id`, so correctness does not depend on every caller.
- Add cross-tenant tests to the existing authorization matrix: tenant A's admin
must be denied on every scope variant belonging to tenant B.

```bash
grep -rn "check_permission(" pangolin/pangolin_api/src/ | grep -v "fn check_permission" | wc -l
```

### A2. `Action::implies` and `Scope::covers` have no adversarial tests

A1's fix rests on `grant.scope.covers(scope)`. The permission matrix tests
confirm allowed things are allowed; they do not systematically confirm that a
narrow grant fails to cover a broad scope. Add the negative direction.

---

## Bucket 2 — Release integrity

The 0.8.0 release published three images, four defects, and a tag that does not
match what shipped. Each item here closes one specific hole that actually opened.

### R1. CI never builds two of the three images

The `docker` job builds `pangolin-api:ci`, starts it, probes shutdown, and fails
if it runs as root. That is a genuine test — and it is the *only* image CI
touches. `Dockerfile.tools` and `pangolin_ui/Dockerfile` are never built in CI at
all, which is exactly why all four 0.8.0 defects were in those two images.

Extend the `docker` job to build and exercise all three:

| Image | Assertion |
|---|---|
| CLI | `pangolin-admin --version` and `--help` exit 0; runs non-root; no `/usr/include/openssl` |
| UI | serves `HTTP 200`; runs non-root; `node_modules` contains no build toolchain |

The UI check is the one that matters most — `npm prune --omit=dev` builds clean
and fails at runtime if a runtime dependency is misfiled as a devDependency.

### R2. Nothing verifies that published artefacts match the tag

Cause of [the 0.8.0 tag/image drift](docs/known-issues/v0.8.0-tag-image-drift.md).
The release must refuse to publish when the working tree differs from the tag
being released, and record the source commit in an OCI label
(`org.opencontainers.image.revision`) so any published image can be traced back.

Resolves the drift as a side effect: 0.9.0's tag and images will agree.

### R3. Images are published from a laptop

`scripts/build_docker_sequential.sh` runs by hand. The 0.8.0 run failed midway
and left the release half-published, and a stray `buildx` process survived a
`pkill` and had to be killed by PID before it pushed an image built from
pre-fix source. Move the push into the tag-triggered workflow that already
builds the binaries.

### R4. `--locked` is not enforced everywhere

Added to `Dockerfile.tools` in 0.8.0. `pangolin/Dockerfile` and the CI build
steps should match, so a published binary is always reproducible from the
committed `Cargo.lock`.

---

## Bucket 3 — Multi-replica correctness

Both of these are documented limitations rather than bugs. They become bugs the
moment someone scales past one replica, which the Helm chart makes easy.

### M1. Rate limiting is per-process

`pangolin_api/src/rate_limit.rs` uses an in-process `moka` cache, so N replicas
give an effective limit of N × the configured value. Documented in the upgrade
guide, but a brute-force limit that silently weakens with scale is the wrong
default. Needs shared backing, or a documented refusal to support it.

### M2. OAuth requires session affinity

The PKCE verifier is held in process — deliberately, since `state` travels
through the browser with the authorization code. Correct for security, but it
means an OAuth login breaks without sticky sessions. Same fix as M1: a shared
store for pending logins.

---

## Bucket 4 — Backend parity

### P1. 59 store-trait methods default to "Operation not supported"

```bash
grep -c "Operation not supported by this store" pangolin/pangolin_store/src/lib.rs
```

Every one is a method some backend may not implement, failing at runtime rather
than compile time. This is how the cloud-credential features shipped without
ever compiling. Audit all 59: which are genuine "this backend cannot do that",
and which are unfinished work wearing the same error message. The parity suite
should assert the intended answer per backend.

### P2. MongoDB branch-create-by-copy is not transactional

PostgreSQL and SQLite got real transactions in 0.8.0; MongoDB falls back to
sequential statements and returns `500` naming the branch. Now that the
replica-set CI job exists, MongoDB transactions are testable.

### P3. `commitTransaction` remains unimplemented

Deliberate, and the only Iceberg REST endpoint still missing. Decide whether
0.9.0 implements it or documents it as permanently out of scope.

---

## Bucket 5 — Debt with a ratchet already in place

Both budgets are enforced in CI and both should move down in 0.9.0.

| Ratchet | Now | Target |
|---|---|---|
| `pangolin/clippy-warning-budget.txt` | 30 (from 314) | 0 |
| `pangolin_ui/svelte-check-budget.txt` | 150 | materially lower |

### D1. Svelte 5 runes: 0 of 90 components

```bash
grep -rl '\$state\|\$derived\|\$props' pangolin_ui/src --include='*.svelte' | wc -l # 0
find pangolin_ui/src -name '*.svelte' | wc -l # 90
```

The UI runs on Svelte 5 in legacy compatibility mode. It works, and it is a
deprecation clock. 0.8.0 called this migration complete — that was accurate for
*compiling and running* under Svelte 5, but no component uses the new reactivity
model. Migrate in tranches with the `svelte-check` budget ratcheting down.

### D2. Ten remaining `TODO`/`FIXME` markers

```bash
grep -rn "TODO\|FIXME" --include="*.rs" --include="*.svelte" pangolin/ pangolin_ui/src | grep -v /target/ | wc -l
```

A1 is one of them. Triage the rest: fix, convert to a tracked item, or delete.
Notable: `user_handlers.rs:681` (token invalidation), `merge_handlers.rs:281`
(`merge_branch` does not return a commit ID), `oauth_handlers.rs:635` (config
loaded from neither environment nor file).

---

## Bucket 6 — Operational hygiene

### O1. `.env` holds `PANGOLIN_ROOT_PASSWORD` in plaintext

Compose passes it into containers. Flagged during the 0.8.0 audit and not acted
on, because changing it is a decision about how operators are expected to run
the thing, not a bug fix. 0.9.0 should decide: secret file, external secret
manager, or an explicit documented statement that this is the supported way.

### O2. GitHub OAuth cannot be OIDC-validated

GitHub issues no `id_token` and publishes no JWKS, so its logins rest on the
userinfo endpoint while every other provider gets full validation.
`PANGOLIN_OIDC_REQUIRE=true` refuses it. This is a permanent property of the
provider — the work is making the asymmetry obvious in the UI, not just the
docs.

### O3. Publish the GHSA

Requires a person. A fixed version now exists on all three channels, which was
the precondition.

---

## Explicitly not in 0.9.0

- **Re-cutting `v0.8.0`.** Replacing a published tag is the one thing a version
number exists to prevent. See the known-issue document.
- **1.0.0.** Not until A1 is resolved and the parity audit in P1 is finished.
A multi-tenant catalog should not call itself 1.0 with an unverified
cross-tenant authorization path.

---

## How to judge 0.9.0 complete

The standard that caught everything worth catching in 0.8.0: **an item is done
when something executes it, not when something reads it.**

Seven times in 0.8.0 a change compiled, passed all 18 CI jobs, and did not work
— cloud-credential features that had never compiled, a cleanup job that never
started, a release pipeline that never released, two guardrail jobs that never
ran a test, a server that killed itself 25 seconds after boot, a load harness
that understated latency by 1000×, and four defects in two container images that
CI never built. Every one was found by running the artefact.

So: no item above is complete on the strength of a green tick alone. R1 exists
precisely because a green tick was not enough.
18 changes: 17 additions & 1 deletion STATUS.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,11 @@ the build's exit code: the CLI reports `pangolin-admin 0.8.0` and runs as uid
10001, the UI serves `HTTP 200` as uid 1000 with a 2.3MB `node_modules`.

Publishing them turned up four defects that every one of the 18 CI jobs had
passed over, because CI builds images and never runs what it built:
passed over. The reason is narrower than "CI does not test images": the
`docker` job builds the **API** image, starts it, probes its shutdown grace and
fails if it runs as root. It is a real test. But it is the only image CI touches
— `Dockerfile.tools` and `pangolin_ui/Dockerfile` are never built in CI at all,
so nothing exercised them until a human ran the release script:

| Defect | Consequence |
|---|---|
Expand All @@ -164,6 +168,18 @@ predates every security fix listed above.
Still requiring a person: decide whether to publish a GHSA now that a fixed
version exists.

## Next

[ROADMAP_0.9.0.md](ROADMAP_0.9.0.md) lists the outstanding work, ordered by what
would hurt most if left undone. The top item is an unverified cross-tenant
authorization path (`authz.rs:39`), and the second is that CI never builds two of
the three container images — the reason all four 0.8.0 image defects survived a
green pipeline.

One known discrepancy is accepted and documented rather than fixed:
[the `v0.8.0` tag and the `0.8.0` image differ](docs/known-issues/v0.8.0-tag-image-drift.md)
by a single CLI help flag.

## If you are deciding whether to run this

The honest summary: the security holes found in the audits are fixed and there
Expand Down
2 changes: 2 additions & 0 deletions docs/known-issues/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,5 @@ This section documents verified issues, limitations, and architectural quirks pr

No verified issues are open against 0.8.0 beyond the limitations recorded in
[STATUS.md](../../STATUS.md), which is the authoritative list.

- [The `v0.8.0` tag and the `0.8.0` container image differ](v0.8.0-tag-image-drift.md) — `--version` is missing from the released CLI binaries but present in the image. Cosmetic; resolved in 0.9.0.
58 changes: 58 additions & 0 deletions docs/known-issues/v0.8.0-tag-image-drift.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# The `v0.8.0` tag and the `0.8.0` container image differ

**Status:** known, accepted, scheduled for 0.9.0
**Affects:** `pangolin-admin` and `pangolin-user` only
**Impact:** cosmetic — a missing `--version` flag. No functional, data or
security difference.

## What differs

`--version` was added to both CLIs *after* the `v0.8.0` tag was cut, and the
container images were then built from that newer commit. So one version number
describes two slightly different builds:

| Artifact | `pangolin-admin --version` |
|---|---|
| GitHub release `v0.8.0` binaries | `error: unexpected argument '--version' found` |
| `alexmerced/pangolin-cli:0.8.0` | `pangolin-admin 0.8.0` |

Verifiable directly: `command(version)` appears zero times at `git show
v0.8.0:pangolin/pangolin_cli_admin/src/main.rs` and once on `main`.

Everything else — the server, the SDK, the UI, the API image, every security
fix in 0.8.0 — is identical across both. The divergence is one clap attribute
on two CLI binaries.

## Why it happened

Publishing 0.8.0 failed partway: the CLI image would not compile because
`Dockerfile.tools` still pinned `rust:1.88`. Fixing that meant running the CLI
image for the first time, which surfaced three further defects, one of them the
missing `--version`. Those were fixed and the images rebuilt — but the tag had
already been cut and the GitHub release binaries already published.

The mistake was sequencing, not the fixes: a defect found after tagging should
either be held for the next version, or the tag should not yet exist.

## Why it was not corrected in place

Re-cutting `v0.8.0` would replace a published tag with different content, which
is the single thing a version number exists to prevent. The release script
already refuses the equivalent for container images
(`scripts/build_docker_sequential.sh` will not overwrite a published tag without
`ALLOW_OVERWRITE=1`), and the tooling should not hold a stricter standard than
the humans using it.

Given the impact is a missing help flag, spending a `0.8.1` release on it was
judged not worth it either.

## Resolution

0.9.0 makes the two consistent, and adds the guard that would have caught it:
the release must verify that the artefacts it publishes were built from the tag
it is publishing. See [the 0.9.0 roadmap](../../ROADMAP_0.9.0.md), item R2.

## If you are affected

Use the container image if you need `--version`, or read the tag you pulled.
`pangolin-admin --help` works identically in both.
Loading