UN-4016 [FEAT] Resolve the organisation from a platform API key via a whoami endpoint - #2269
Conversation
…endpoint sends Iteration 1 of unstract:remediation against PR #2269. Fifteen of sixteen findings; F6 is handed back (see below). The two High findings were both invisible from inside the change: F1 — the spec published `ErrorResponse` for 401/403, but `whoami` is deliberately not whitelisted, so `CustomAuthMiddleware` answers every rejection itself with a bare `{"message": ...}` and DRF's exception handler is never reached. A generated client branching on `errors[0].code` would raise on the most common failure the endpoint has. Now declares a `PlatformKeyError` shape that matches the wire, with the 500 carrying no body schema because a Django HTML 500 has none. F2 — `@pytest.mark.critical_path("platform-key-whoami")` named an id in no registry, and `tests/rig/cli.py` sets `overall_exit = 1` on an unknown marker. The rig would have failed the build on every run. CI never caught it because `test` skips on draft PRs. Now registered. Also fixed: the view returned 403 where the spec said 401 (DRF coerces `NotAuthenticated` unless the first authenticator offers a WWW-Authenticate header, and SessionAuthentication offers none) — it now returns 401 explicitly, and the branch has a test, which it never had because every other rejection is answered before the view runs. The whitelist regex is anchored, so an organisation named `whoami` no longer has its whole API treated as organisation-less. The organisation-scoped alias `/<org>/whoami/`, which the mount comment wrongly claimed could not exist, is now documented and tested in both directions. The dominant defect class was not any single bug: seven of sixteen findings were confidently-worded comments asserting mechanisms the code does not implement — that DRF resolves no user, that the org FK is non-null by construction, that no rewrite can produce `whoami/`, that a test observes middleware it never invokes. Each is corrected to what the code does, or deleted. Every fix is mutation-checked: reverting it fails a named test. That check also caught one of the new tests passing vacuously — it matched an error string the OpenAPI-validity gate produces, so it went green with the gate it was written for removed. F6 is not fixed here and needs a decision: setting `organization_id = None` makes `whoami` bypass `SubscriptionMiddleware` in the enterprise tree, where every other org-less path has an explicit `SUBSCRIPTION_WHITELISTED_PATHS_LIST` entry. The fix belongs in unstract-cloud and the intent is a product call. Findings: F1 F2 F3 F4 F5 F7(partial) F8 F9 F10 F11 F12 F13 F14 F15 F16 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM
|
@chandrasekharan-zipstack @hari-kuriakose — one finding from this PR's review needs a decision from you, because the fix isn't in this repo.
|
…the default points Iteration 1 of unstract:remediation against PR #3. Nineteen findings across fifteen classes; one escalated (below). The headline defect had four faces and one cause. `config.py` stated a relationship in a comment -- "the same host as docstudio: one deployment serves both the platform API and the deployments it manages" -- and implemented it as a constant on the very next line. So `platform.base_url` ignored a profile written before the `platform` block existed, ignored `docstudio --base-url`, and left `--api-key` inert for `deployment ls`, sending an organisation-admin platform key to `us-central.unstract.com` after the operator had explicitly named their own host. A reviewer proved it by pointing `--base-url` at 127.0.0.1:9 and getting a real 401 back: a closed port cannot answer, so the request reached the SaaS default. `platform_base_url()` now falls back to the resolved docstudio host, which fixes all four sites at once. The security scan rates it 3/10 as a vulnerability -- vendor host, TLS, and `requests` strips the auth header across hosts -- so it lands as a correctness defect, not a leak. The other four High findings: - `_store_organisation` re-derived the profile ladder and dropped the `$UNSTRACT_PROFILE` tier, so the key resolved from one profile and the org was written into another; the next command then failed after a `whoami` that reported `saved: true`. It now takes `ResolvedConfig.active_profile`, the same chain every read uses. It also refuses to *create* a profile that is not in the file: `setdefault` was materialising a typo, permanently disarming the "Profile not found" guard so every later command silently resolved production defaults. - A failed config write threw away an identity the network call had already returned, exiting 1 ("check your disk") or 2 ("usage error") with `data: null`. `ExitCode.SAVE_FAILED` exists for exactly this and `poll.py` already uses it; the identity now reaches stdout in `details` either way. - `config init` wrote `api_key = "env:UNSTRACT_PLATFORM_KEY"` into every starter profile, and an `env:` reference to an unset variable is a `config doctor` problem -- so doctor exited 1 for every user without a platform key, which this PR's own comment calls the common case. The key is dropped from the starter blocks. - `core/platform.py` -- the only wire-facing new code -- had no test executing it at all, because every command test replaces the factory. Two mutations (breaking the whoami URL, forcing every listing to organisation "") left the suite green. `tests/test_platform.py` now exercises the real class; both mutations fail it. Also fixed: a 204 or non-dict body raised an AttributeError that matched no arm in `__main__`, so the caller got a traceback and no envelope -- the one thing this CLI promises never to do; `requests` transport errors (a scheme-less base_url, a proxy's HTML on a 200) reached the entry point's full-disk handler and were reported as "Check the path and disk."; `PlatformAPIError` folded up to 2KB of server body into `error.message`, which `emit_error` documents as a one-line summary; `--transport-timeout` was accepted on `deployment ls` and ignored (8.65s elapsed against a 1s flag), and `auth` had no such flag at all; `api_path_prefix` was hard-wired, so whoami and ls were unreachable on precisely the self-hosted installs the onprem-example profile caters to, while `clone` worked; `whoami --save` rewrote a discovered project-local `.unstract.toml`, dropping its comments and narrowing its mode; the write was invisible outside `-o json`; the 401 hint talked about deployments on a command that has none; and the README claimed a deployment key "runs one deployment", contradicted by three other statements including this file's own KEY_SOURCES. Every fix is mutation-checked: reverting it fails a named test, nine of nine. ESCALATED, needs a decision: `GET /api/v1/unstract/whoami/` is served only by Zipstack/unstract#2269, which is unmerged. Against any released Unstract the README's documented first command 404s, and `hint_for(404)` sends the reader after a resource id that does not exist. `config doctor --probe` inherits it and exits non-zero on a good setup. Whether this CLI ships before the backend, and what it should say when it does, is a release call rather than a fix. Findings: A B C D E F G H I J K L M N O(escalated) P Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM
 |
hari-kuriakose
left a comment
hari-kuriakose
left a comment
There was a problem hiding this comment.
@pk-zipstack Why do we even need to enumerate org from the API key in the client? When you pass a platform API key, the corresponding org id should be retrieved in the backend automatically.
|
@hari-kuriakose You're right, and the code backs you more strongly than I'd assumed when I wrote this. I went and checked before answering. The org id in the URL is not a source of truth anywhere in this middleware — it is only an assertion. The org that actually scopes a request comes from the credential in both auth paths:
And everything downstream reads that, not the URL: The only use of the URL org is the mismatch check at What I think that means The cleaner design is the one you're pointing at: let a platform-key-authenticated request omit the org segment and resolve it from the key — which is exactly what this PR's Three things worth deciding rather than assuming, which is why I'm not just doing it:
So, the question back to you and @chandrasekharan-zipstack @Deepak-Kesavan: do you want org-less aliases for the platform-key endpoints as a follow-up — and if so, all of them or just the deployment ones? I'm happy to do that work; I'd rather agree the API surface with you first than guess at it. In the meantime this PR is self-contained and its value doesn't depend on the answer. Happy to hold it if you'd rather land the whole shape at once. Co-authored with Claude Code. Every file and line reference above was verified against |
@hari-kuriakose this is mainly to ease friction of configuring creds for the CLI. Currently, users would have to paste org_id, API keys (both Unstract and LLMW separate) and deployment endpoints to a config file. With this Long-term, supporting OAuth might be an even better experience |
@chandrasekharan-zipstack Sorry, didn't get you still. With just the Platform API key given as input, shouldn't we be able to get a list of all API Deployments then auto provision a Global API Deployment key and proceed from there? Why do we need this new endpoint? OAuth login is required eventually but that is a separate and secure API anyways. |
@hari-kuriakose in order to list the API deployments -> we need both platform API key + org_id (this is because org_id is needed to frame the URL to list deployments).
|
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review — 4 High, 8 Medium
Reviewed against head 245672ae9. Findings are inline. 13 Low findings (mostly comment/doc accuracy and uncovered branches) are omitted here and can be shared separately if useful.
The core design is sound: the org is derived from key.organization, tenant isolation holds on both URL forms, and the alias correctly re-arms the belongs-to-org check. The findings below are about the edges — cloud behaviour, the accuracy of the comments after the last commit, and two tests that do not currently constrain anything.
Checked and clear, so nobody re-derives them:
- The
$anchor on the whitelist regex is correct and does not break the alias — the whitelist runs on the un-rewrittenrequest.path, so the alias correctly fails it and falls through to the rewrite. /api/v1/unstract/whoami(no trailing slash) does not produce a misleading 403. It fails the org pattern entirely (which requires a/after the org segment), soorganization_idstaysNoneandCommonMiddlewareAPPEND_SLASH redirects.- The spec-test narrowing from "every operation" to "deployment operations" is net stronger, not weaker — the added else-branches constrain
whoamimore tightly than the original did. - No concurrency concerns:
StateStoreisthreading.local()and cleared in afinally. - No added queries:
select_related("organization")already covers the view's single read.
hari-kuriakose
left a comment
hari-kuriakose
left a comment
There was a problem hiding this comment.
Review — REQUEST-CHANGES · 0 Critical, 1 High, 6 Medium, 3 Low
Reviewed at head 245672ae9 against merge-base c49968e3e (13 files, 895 lines), under the team's standardized 19-lens rubric. Findings are inline.
This is a follow-up to my review of 2026-09-02. Reconciliation of that finding is below.
The core design is sound, and I verified it rather than assuming it: tenant isolation holds. The view reads key.organization, and custom_auth_middleware.py:140 stamps the StateStore org from the key row, never from the URL. The endpoint genuinely authenticates — it is deliberately absent from WHITELISTED_PATHS, and a test pins that. The alias re-arms the key-belongs-to-org check and is tested on the wire, body included. The test suite is well above average for this repo: it asserts response bodies rather than statuses alone, and its docstrings are candid about what they do not cover.
The change request is driven by the spec-gate regression and the accompanying claim in the PR description, not by the finding count — most of which is comment accuracy.
My prior finding — STILL-OPEN
I asked: "Why do we even need to enumerate org from the API key in the client? When you pass a platform API key, the corresponding org id should be retrieved in the backend automatically."
Reading the code now: the premise was correct. custom_auth_middleware.py:140 already resolves the organisation from the key on every Bearer request, with no client involvement.
What this PR adds is orthogonal to that. Org-scoped routes are mounted under /<org>/, and OrganizationMiddleware parses the organisation out of the path, so a client still needs the string in order to construct those URLs — even though the backend independently knows it. The endpoint therefore solves a real and distinct problem (URL construction).
It does not, however, answer the question I asked: whether org-scoped routes should accept a platform key without the org segment, given the backend already resolves it. That is a larger design decision which this PR neither forecloses nor addresses. Not blocking — recorded so it is not lost.
Unanchored findings
1. The PR description states a property the code no longer has. It says the published-prefix check is "still rejecting an overridden API_DEPLOYMENT_PATH_PREFIX". For any override under the tenant mount it now accepts it — see the inline finding on generate_docstudio_spec.py:33. Worth correcting in the description as well as the code, since this is the artifact reviewers read first.
2. The unstract-cloud subscription-gate question — @praveen-formido asked me by name, so here is an answer rather than a deferral. Exemption is defensible: an identity probe that tells you which org your key belongs to should arguably answer regardless of billing state, and it reads nothing billable. But it should be an explicit SUBSCRIPTION_WHITELISTED_PATHS_LIST entry with a comment like its neighbours (marketplace/webhook/, tackle/webhook/, …), not a side effect of organization_id being None — that breaks the day get_subscription stops tolerating a null org, and it silently issues a Subscription.objects.get(organization_id=None) query on every call. I cannot verify current cloud state from this repo, so this is a recommendation on the decision, not a verification of the code.
3. Lens 13 coverage is split. The whoami suite could not be executed in review — APITestCase + conftest.py:26-44 auto-marks it integration, requiring live Postgres. 17 tests collected, 0 run. The reasoning about the suite is sound and produced a real finding, but no wire behaviour was exercised by this review; CI remains the only evidence there.
Lens checklist (19/19)
| # | Lens | Status |
|---|---|---|
| 1 | Correctness | Clean |
| 2 | Design & fit | See findings |
| 3 | Error handling | Clean — explicit clean report from the specialist |
| 4 | Security | Not covered — deep-dive ran DEGRADED (finder sub-task never returned; ran inline, filter stage vacuous). Surface read directly by me |
| 5 | Data integrity | N/A — no migrations, no model changes |
| 6 | Concurrency | N/A — no primitives added |
| 7 | API compatibility | See findings |
| 8 | Reliability | Clean — no timeouts/retries/external calls added; single in-memory attribute read on an already-select_related row |
| 9 | Performance & cost | Clean — no added queries |
| 10 | Observability | Clean |
| 11 | Operational safety | Clean |
| 12 | LLM/agent | N/A |
| 13 | Tests | Not covered — Postgres unreachable, 17 collected / 0 run (findings still filed) |
| 14 | Dependencies & build | N/A — no dependency or lockfile changes |
| 15 | Guideline compliance | Clean — pinned ruff 0.3.4 check and format --check both pass |
| 16 | Doc accuracy | See findings |
| 17 | Cross-document consistency | Clean — merge-result sweep ran (branch is 8 commits behind main); no merge-created divergence |
| 18 | On-prem experience | N/A — no charts; the setting is absent from every env sample and values schema |
| 19 | Scaling & workload lifecycle | N/A |
boundary-sweep: 5 sinks traced, clean — four response fields plus the 401 literal. The diff adds no log lines at all. Every field is read off the caller's own key row, so the viewer is entitled to it by construction; no key material reaches the response.
Checked and clear — so nobody re-derives them
- The published spec does not contradict itself. I initially suspected
whoami's 401/403 declaredPlatformKeyErrorwhile carrying{type, errors[]}examples. That was my own analysis error — a script that misattributed deployment-operation examples towhoami. Corrected:whoami401/403 carry no examples.PlatformKeyAutoSchemaworks as designed. test_no_published_example_contradicts_its_own_schemais not vacuous. It executes zero assertions today, but that is what a for-all check looks like when its invariant holds. Settled by mutation rather than reading: deletePlatformKeyAutoSchema._get_examples, regenerate, and the whoami 401/403 gain examples, the loop body executes, and the test fails. It binds exactly the property it advertises.- The spec is not stale —
openapi_schema.pyandspecs/docstudio-oss.jsonwere regenerated together in245672ae9. - The spec change is purely additive (112 insertions, 0 deletions), so the deployment operations are byte-identical and no downstream client PR is forced by this change.
- Nothing is shadowed by the new mount — no tenant urlconf declares
whoami/. - The
$anchor is correct, and/api/v1/unstract/whoami(no trailing slash) does not produce a misleading 403. - The
ruffI001 you may see when lintingtest_whoami.pyby explicit path is an exclusion artifact, not a real violation —--force-excludereports "No Python files found", so pre-commit skips it.
Standardized 19-lens review · unstract:standard-review · 7 agents · scoped to git diff c49968e3e04c0cfd0254fd936d09aee14c0eefd8 245672ae9
|
Update — lens 4 (Security) upgraded from When I posted the review above, the security deep-dive's finder sub-task had failed to return, so the pass ran degraded and I reported lens 4 honestly as Result: no findings at confidence >= 8. Nine candidates were enumerated and each fell below the floor with evidence — the newly-live whitelist branch (exactly one runtime consumer, no settings module redefines it), the The substantive point on the guard short-circuit is worth restating, because it is stronger than "the skip is compensated": nothing is skipped that had anything to check. This remains a precision-biased scan, not a clean bill of health: its hard exclusions mean DoS, rate limiting, resource exhaustion and dependency currency were never candidates. The unthrottled-key-oracle observation in my Low findings above sits in exactly that excluded space, so it stands on its own footing rather than being cleared by this scan. One small correction to a comment, surfaced by the re-run.
re.match(r"^/api/v1/unstract/whoami/$", "/api/v1/unstract/whoami/\n") # matches
re.match(r"^/api/v1/unstract/whoami/$", "/api/v1/unstract/whoami/\nfoo") # does notNo security consequence, which is why it is not in the findings list: the newline variant resolves to the same view with the same key-derived organisation, and nothing can be smuggled after the newline. The anchor's actual purpose — stopping an organisation literally named If you want the comment to be exactly true, Follow-up to the standardized 19-lens review above. Lens 4 now reads: Clean — completed find pass, no findings at confidence >= 8, within the scan's stated exclusions. |
|
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…bout them Greptile, on PR #2269. The rejection logging added in 0e9a23a took the first `X-Forwarded-For` value as the caller address. Nothing in this project validates a forwarding chain, so that value is supplied by the party being logged -- which defeats the only purpose those lines have, and puts an unvalidated header into the log stream. `REMOTE_ADDR` only now, matching `internal_api_auth.py` and `internal_base_urls.py`, the two other places that log a caller address. Behind a proxy this records the proxy; recovering the real client needs a trusted-proxy setting this project does not have, and guessing a hop count here would be the same mistake in a different shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ
Review response —
|
| Finding | Resolution |
|---|---|
Spec gate accepted an overridden API_DEPLOYMENT_PATH_PREFIX |
PUBLISHED_PATH_PREFIXES now lists each route, not the mount. Verified: @hari-kuriakose's exact case now fails the gate; unset, the spec regenerates byte-identically |
openapi_schema.py docstring claimed two published error shapes |
Deleted; replaced with the published shape plus an explicit wire/spec separation |
| 403 unreachable on the published path | Withdrawn. Moved to the deployment assertion beside 400/404; universal set is now {401, 500}, which resolves the contradiction between the suite's own two rules |
| Middleware pin hid the ordering the route depends on | Ordering now asserted against the shipped setting, outside the override. Mutation-checked: swap the two entries and all 17 existing tests stay green while only the new assertion fails |
"cloud test settings drop CustomAuthMiddleware" |
False, and corrected. unstract-cloud/…/test_cloud.py:7 says "No middleware is excluded." Chandru was right |
| Whitelist re-spelled the mount, unescaped env value | rf"^/{re.escape(TENANT_SUBFOLDER_PREFIX)}/whoami/$" |
| Example-divergence allow-list coarse and un-self-checking | Keyed by schema name; asserts its entries still match. It immediately caught three wrong schema names in my own first draft |
| That check ran zero assertions | Positive case added |
MCP organization holds an id, not a name |
Documented, with the direction of the trap |
Missing WWW-Authenticate on 401 |
Added, spelled as mcp_server/transport.py:98 |
| Whitelist disarms both org guards | Noted at the setting and at custom_auth_middleware.py:59 — the session guard my prose never mentioned |
| No logging on rejection branches | WARNING on both, plus the cross-tenant 403, with a truncated SHA-256 fingerprint rather than the token |
Left open deliberately — three threads, all needing your call rather than more code from me:
- The cloud 402 divergence. Cross-repo; I will raise the
unstract-cloudPR adding"unstract/whoami/"toSUBSCRIPTION_WHITELISTED_PATHS_LIST. You both reached the same remedy independently. request.organization_from_urlinstead of a falsyorganization_id. I think Chandru is right that it is the better design, but it changes the condition on a shared security control for every authenticated route. That wants its own PR and its own reviewers.ScopedRateThrottleand collapsing the two 401 bodies. The logging half is done. The throttle needs a rate nobody has chosen, on the endpointconfig doctor --probecalls; and collapsing the messages changes behaviour for every Bearer route. Both are decisions, not omissions.
On evidence: @hari-kuriakose noted his review collected 17 whoami tests and ran 0, so no wire behaviour was exercised. I stood up Postgres and ran them — 58 pass, ruff 0.3.4 check and format clean. The two guards I claim above are mutation-checked, not asserted.
Greptile then caught a defect in my own fix: the new logging trusted X-Forwarded-For, letting the party being logged choose the address recorded about them. Fixed in 42b695b9f — REMOTE_ADDR only, matching the two other places in this codebase that log a caller address.
I also corrected the PR description, which repeated the spec-gate claim.
Co-authored with Claude Code.
…bout them Greptile, on PR #2269. The rejection logging added in 0e9a23a took the first `X-Forwarded-For` value as the caller address. Nothing in this project validates a forwarding chain, so that value is supplied by the party being logged -- which defeats the only purpose those lines have, and puts an unvalidated header into the log stream. `REMOTE_ADDR` only now, matching `internal_api_auth.py` and `internal_base_urls.py`, the two other places that log a caller address. Behind a proxy this records the proxy; recovering the real client needs a trusted-proxy setting this project does not have, and guessing a hop count here would be the same mistake in a different shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ
42b695b to
9bad32a
Compare
|
recheck |
hari-kuriakose
left a comment
hari-kuriakose
left a comment
There was a problem hiding this comment.
@praveen-formido LGTM overall.
…endpoint sends Iteration 1 of unstract:remediation against PR #2269. Fifteen of sixteen findings; F6 is handed back (see below). The two High findings were both invisible from inside the change: F1 — the spec published `ErrorResponse` for 401/403, but `whoami` is deliberately not whitelisted, so `CustomAuthMiddleware` answers every rejection itself with a bare `{"message": ...}` and DRF's exception handler is never reached. A generated client branching on `errors[0].code` would raise on the most common failure the endpoint has. Now declares a `PlatformKeyError` shape that matches the wire, with the 500 carrying no body schema because a Django HTML 500 has none. F2 — `@pytest.mark.critical_path("platform-key-whoami")` named an id in no registry, and `tests/rig/cli.py` sets `overall_exit = 1` on an unknown marker. The rig would have failed the build on every run. CI never caught it because `test` skips on draft PRs. Now registered. Also fixed: the view returned 403 where the spec said 401 (DRF coerces `NotAuthenticated` unless the first authenticator offers a WWW-Authenticate header, and SessionAuthentication offers none) — it now returns 401 explicitly, and the branch has a test, which it never had because every other rejection is answered before the view runs. The whitelist regex is anchored, so an organisation named `whoami` no longer has its whole API treated as organisation-less. The organisation-scoped alias `/<org>/whoami/`, which the mount comment wrongly claimed could not exist, is now documented and tested in both directions. The dominant defect class was not any single bug: seven of sixteen findings were confidently-worded comments asserting mechanisms the code does not implement — that DRF resolves no user, that the org FK is non-null by construction, that no rewrite can produce `whoami/`, that a test observes middleware it never invokes. Each is corrected to what the code does, or deleted. Every fix is mutation-checked: reverting it fails a named test. That check also caught one of the new tests passing vacuously — it matched an error string the OpenAPI-validity gate produces, so it went green with the gate it was written for removed. F6 is not fixed here and needs a decision: setting `organization_id = None` makes `whoami` bypass `SubscriptionMiddleware` in the enterprise tree, where every other org-less path has an explicit `SUBSCRIPTION_WHITELISTED_PATHS_LIST` entry. The fix belongs in unstract-cloud and the intent is a product call. Findings: F1 F2 F3 F4 F5 F7(partial) F8 F9 F10 F11 F12 F13 F14 F15 F16 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM
…bout them Greptile, on PR #2269. The rejection logging added in 0e9a23a took the first `X-Forwarded-For` value as the caller address. Nothing in this project validates a forwarding chain, so that value is supplied by the party being logged -- which defeats the only purpose those lines have, and puts an unvalidated header into the log stream. `REMOTE_ADDR` only now, matching `internal_api_auth.py` and `internal_base_urls.py`, the two other places that log a caller address. Behind a proxy this records the proxy; recovering the real client needs a trusted-proxy setting this project does not have, and guessing a hop count here would be the same mistake in a different shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ
9bad32a to
3a0da6a
Compare
… whoami endpoint
A caller holding a platform API key knows the secret but not what it is scoped
to: `organization_id` is only discoverable by reading it out of a web-app URL,
and every organisation-scoped endpoint takes it as a path segment. This adds
`GET /api/v1/unstract/whoami/`, which returns `{organization_id,
organization_name, permission, key_name}` read off the key row itself.
The route carries no organisation segment, and that is what made it more than a
view. `OrganizationMiddleware` matches `^/api/v1/unstract/<org>/`, so it parsed
`whoami` as the organisation and rewrote the path to a 404. Its whitelist escape
hatch then returned without ever setting `request.organization_id`, which
`CustomAuthMiddleware` reads by bare attribute access -- a 500 rather than a
skip. Both are fixed: the path is whitelisted for the organisation middleware
only (it still authenticates), and the whitelist branch now sets the attribute,
which protects any future organisation-less path.
The endpoint also flows through the committed OpenAPI spec, which needed three
gates widened: the published-prefix check now accepts the tenant mount as well
as the deployment one, `SPEC_URLCONFS` gains the new urlconf, and the two spec
tests that looped over every operation asserting deployment-specific facts now
pin those facts to the deployment operations and check only genuinely universal
ones globally. The regenerated spec is +160/-0 -- the deployment contract is
byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM
…endpoint sends Iteration 1 of unstract:remediation against PR #2269. Fifteen of sixteen findings; F6 is handed back (see below). The two High findings were both invisible from inside the change: F1 — the spec published `ErrorResponse` for 401/403, but `whoami` is deliberately not whitelisted, so `CustomAuthMiddleware` answers every rejection itself with a bare `{"message": ...}` and DRF's exception handler is never reached. A generated client branching on `errors[0].code` would raise on the most common failure the endpoint has. Now declares a `PlatformKeyError` shape that matches the wire, with the 500 carrying no body schema because a Django HTML 500 has none. F2 — `@pytest.mark.critical_path("platform-key-whoami")` named an id in no registry, and `tests/rig/cli.py` sets `overall_exit = 1` on an unknown marker. The rig would have failed the build on every run. CI never caught it because `test` skips on draft PRs. Now registered. Also fixed: the view returned 403 where the spec said 401 (DRF coerces `NotAuthenticated` unless the first authenticator offers a WWW-Authenticate header, and SessionAuthentication offers none) — it now returns 401 explicitly, and the branch has a test, which it never had because every other rejection is answered before the view runs. The whitelist regex is anchored, so an organisation named `whoami` no longer has its whole API treated as organisation-less. The organisation-scoped alias `/<org>/whoami/`, which the mount comment wrongly claimed could not exist, is now documented and tested in both directions. The dominant defect class was not any single bug: seven of sixteen findings were confidently-worded comments asserting mechanisms the code does not implement — that DRF resolves no user, that the org FK is non-null by construction, that no rewrite can produce `whoami/`, that a test observes middleware it never invokes. Each is corrected to what the code does, or deleted. Every fix is mutation-checked: reverting it fails a named test. That check also caught one of the new tests passing vacuously — it matched an error string the OpenAPI-validity gate produces, so it went green with the gate it was written for removed. F6 is not fixed here and needs a decision: setting `organization_id = None` makes `whoami` bypass `SubscriptionMiddleware` in the enterprise tree, where every other org-less path has an explicit `SUBSCRIPTION_WHITELISTED_PATHS_LIST` entry. The fix belongs in unstract-cloud and the intent is a product call. Findings: F1 F2 F3 F4 F5 F7(partial) F8 F9 F10 F11 F12 F13 F14 F15 F16 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM
Iteration 2. The adversarial verifier re-found F1: iteration 1 changed the
401/403 `$ref` to `PlatformKeyError` but left the `examples` block beside it
showing the handler's `{type, errors[]}` body, so the artifact contradicted
itself in a single media-type object -- schema requiring `message`, example
showing `errors[0].code`. A client author reads the example, not the `$ref`, so
the defect F1 was filed against survived its own fix.
Cause: `drf_standardized_errors` appends an example of the handler body to
every 4xx/5xx keyed on the status code alone, never consulting the declared
serializer (`openapi.py:343-356`), so overriding `responses` was never going to
be enough. `PlatformKeyAutoSchema` suppresses that injection for this view
only; the deployment operations keep their examples, which are correct there
because those really do return the handler body.
Also fixed, all four raised by the same verifier against iteration 1's own work:
the module docstring claimed the middleware answers *every* rejection, which the
401 iteration 1 added to the view had just made false; the new spec test lacked
the `assert reads` guard that the same commit added twelve lines below it, so it
passed vacuously on an operationId rename; the alias test's docstring claimed
mount-order coverage a mutation disproved (swapping the mount leaves every test
green); and the 403 body was declared but asserted nowhere, which is the exact
"status asserted, body unasserted" gap that let the original spec lie.
`test_no_published_example_contradicts_its_own_schema` now pins the property
structurally for every operation. It found three pre-existing instances in the
merged deployment spec -- `status` 406/500 and `execute` 500 all declare a
non-error body beside a handler-shaped example. Those are recorded in
`_KNOWN_EXAMPLE_DIVERGENCES` rather than fixed here: they predate this branch
(verified against c49968e) and belong to whoever owns that spec. The check
fails on any new instance.
Findings: F1(re-fixed) N2 N3 N4 N5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM
…claiming it cannot
Iteration 3. The verifier re-found N2 with a counter-example rounds 1 and 2
both missed: `POST /api/v1/unstract/whoami/` with a `read_write` key -- the
model default -- is passed by the middleware, refused by DRF, and comes back
405 in the exception handler's `{type, errors[]}`. Four comments across three
files asserted that shape was unreachable here, and that claim was the stated
justification for both not reusing `ErrorResponse` and suppressing every error
example on the view.
So the endpoint has always returned two error shapes and the spec described
one. 405 is now declared with `ErrorResponse`, and `PlatformKeyAutoSchema`
narrows its suppression to the statuses the middleware actually answers (401,
403) rather than blanket-stripping, so the 405 keeps the handler example that
is correct for it. Verified on the wire: a `read_write` key POSTing gets 405
`{type, errors[]}`; a `read` key gets 403 `{message}` from the middleware
instead. Both are now pinned, against each other, so neither path can look like
the only one.
The absolute clauses are deleted rather than rewritten a third time. Two
rounds of rewriting them produced two more false statements; what remains says
only what is enforced.
Also from the same verifier: `_KNOWN_EXAMPLE_DIVERGENCES` was keyed on
`(operationId, code)`, which exempted those coordinates forever -- a new
divergent example injected at `status` 406 passed. Now keyed on the example
name too, so the three recorded rows stay exempt and a fourth fails; the
comment claiming the check catches new instances is true as written for the
first time. Dropped a dead exemption branch that could never match, and a
comment misattributing which commit added a guard and how far below it sits.
Findings: N2(re-fixed) + 4 new from verifier round 2
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM
for more information, see https://pre-commit.ci
…not return Iteration 4, and the last: N2 is escalated rather than attempted a fourth time. Round 3 declared 405 under the `get` operation. A verifier's 42-cell matrix (3 tiers x 7 methods x 2 routes) shows GET returns 200, 401 or 403 and never 405 -- so the declaration was unreachable at the coordinate it was written, and the 405s that do occur (POST/PUT/PATCH at read_write+, DELETE at full_access) have no operation in the spec to attach to. The kept example made it worse: it published `Method "get" not allowed.` under the GET operation, a hardcoded placeholder from drf_standardized_errors, not derived from the route. The declaration is withdrawn. The behaviour is real and is now stated in the operation description in prose, which is where a fact about methods the route does not serve can honestly live. Also corrected, all introduced by round 3 and all found by the same verifier: an I001 failure of the repo's PINNED ruff 0.3.4 hook, which round 3 introduced and the installed ruff 0.15 does not report -- verified clean at 7eb41bc and failing at 32ec142, then confirmed the fix satisfies both versions; a comment asserting everything else "keeps the handler shape", contradicted by the 500 declared eight lines below it; a true clause round 3 deleted while rewriting the sentence around it, restored verbatim; and a test whose name claimed a relationship to the spec that nothing in it tested -- renamed to what it actually asserts, with the gap it does not cover named in its docstring. WHAT IS ESCALATED, and why this loop stops here: N2 has survived three fix attempts. Each attempt was a correct reading of the previous failure and each introduced a new defect: round 2 rewrote a false claim into a stronger false one, round 3 fixed the claim and declared the status in the wrong place. Three consecutive rounds introducing new defects is the convergence tripwire, and the discriminator applies -- the fixes keep needing an exception to a shared rule, which means the rule is wrong, not the wording. The rule that is wrong: this path serves GET, but answers non-GET methods with a status and a body shape that no `get` operation can describe. OpenAPI attaches responses to operations, not paths. Options, none of which a remediation loop should pick unilaterally: declare stub operations for the methods purely to carry a 405; accept prose (what this commit does); or change the view so every method the tier permits is answered in one shape. Findings: NEW-1..NEW-5 from verifier round 3 fixed; N2 ESCALATE (attempts=3) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM
**Spec gate (the change request).** `PUBLISHED_PATH_PREFIXES` carried
`api/v1/unstract` -- exactly `TENANT_SUBFOLDER_PREFIX` -- and the check is a
`startswith` over the union, so an `API_DEPLOYMENT_PATH_PREFIX` pointed anywhere
under the tenant mount passed a gate whose own comment says it exists so such an
override fails. Each entry is now the route rather than the mount it hangs off.
Verified: with `API_DEPLOYMENT_PATH_PREFIX=api/v1/unstract/deploy` the generator
now fails; unset, it regenerates byte-identically.
**403 withdrawn from the published operation.** Every route to one is closed on
`/api/v1/unstract/whoami/`: the belongs-to-org guard is skipped, `allows` admits
GET at every tier, and an unknown tier is barred by migration 0003's check
constraint. It moves to the deployment-specific assertion alongside 400/404,
where 403 is genuinely reachable; the universal set becomes {401, 500}. This
resolves the contradiction between that universal rule and the dead-branch rule
the same suite states.
**Middleware ordering is now asserted against the shipped setting.** The suite
pins MIDDLEWARE, which hid the one ordering this route depends on -- reverse
TENANT and CUSTOM_AUTH in settings and all 17 tests stayed green while every
whoami request would AttributeError in production. Verified by doing exactly
that: only the new assertion fails.
Also corrected: the docstring claiming the operation publishes two error shapes
(it publishes one, and `ErrorResponse` is not even imported); "one shape covers
every rejection", which the declared 500 contradicts; the whitelist entry
re-spelling a mount the file already derives, now built from
`TENANT_SUBFOLDER_PREFIX` with `re.escape` since `PATH_PREFIX` is environment
data landing in a regex; and the false claim that cloud test settings drop
`CustomAuthMiddleware` -- `unstract-cloud`'s `test_cloud.py` says the opposite in
as many words.
Added: `WWW-Authenticate` on the 401 (RFC 9110 s11.6.1), spelled as in
`mcp_server/transport.py`; WARNING logs on the two credential-rejection branches
and the cross-tenant 403, with a truncated SHA-256 fingerprint rather than the
token, so "was this key ever presented?" has data to answer from; a note that
MCP's `organization` holds an id, not a name, so nobody aligns the two surfaces
by field name and ships an id where a display name belongs; and a note in
settings that an entry in this whitelist disarms both organisation guards.
The example-divergence allow-list is now keyed by schema name and asserts its
entries still match, so a fixed divergence fails until its entry is deleted --
which immediately caught three wrong schema names in my own first draft of it.
Its check ran zero assertions against the committed spec, so a positive case now
proves it fires.
58 tests pass against live Postgres, ruff 0.3.4 check and format clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ
@hari-kuriakose asked for this specifically: the PR's prose reasoned carefully about the Bearer branch's belongs-to-org check and never mentioned that the session branch's org-access-denied check is conditioned on the same attribute, so a path added to ORGANIZATION_MIDDLEWARE_WHITELISTED_PATHS loses both. The next reader looks here, not at the settings file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ
…bout them Greptile, on PR #2269. The rejection logging added in 0e9a23a took the first `X-Forwarded-For` value as the caller address. Nothing in this project validates a forwarding chain, so that value is supplied by the party being logged -- which defeats the only purpose those lines have, and puts an unvalidated header into the log stream. `REMOTE_ADDR` only now, matching `internal_api_auth.py` and `internal_base_urls.py`, the two other places that log a caller address. Behind a proxy this records the proxy; recovering the real client needs a trusted-proxy setting this project does not have, and guessing a hop count here would be the same mistake in a different shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ
3a0da6a to
bf8bfaf
Compare
|
Unstract test resultsPer-group results
Critical paths
|
4 participants
What
GET /api/v1/unstract/whoami/— an organisation-less endpoint that authenticates with a platform API key and answers{organization_id, organization_name, permission, key_name}, read off the key row itself. It reaches the generated OpenAPI spec, so the published client and the CLI pick it up.Builds on the spec pipeline from #2237 (UN-4009), which merged on 31 Aug — this was written against that branch and has been rebased onto
mainsince, with the spec regenerated and re-verified against the merged result.Why
Before a caller can use any organisation-scoped endpoint they need
org_id, and it has no documented source — it is the first path segment of every web-app URL (useMainAppRoutes.js,<Route path=":orgName">) and nothing says so. A platform key already carries its organisation by construction:PlatformApiKeyinheritsDefaultOrganizationMixin, whose FK is stamped at mint time from the minting admin's active org, andkeyis globally unique, so a bearer token maps to exactly one row and therefore one organisation. Nothing exposed that association over HTTP.Every comparable CLI that predates OAuth (
doctl,twilio,sentry-cli) issues one opaque token that carries its own scope and is validated the moment it is supplied. This is the endpoint that lets ours do the same.How
platform_api/whoami_views.py— a field read.permission_classes = []becauseCustomAuthMiddlewarehas already resolved the token to a key row, bound the service account torequest.userand enforced the tier against the method; a permission class would re-ask a question already answered.authentication_classesis deliberately not set — the project configures noDEFAULT_AUTHENTICATION_CLASSESthat resolves a user, so DRF's default returnsNoneand the middleware'srequest.usersurvives into the view. No extra query:custom_auth_middleware.pyalreadyselect_relatedsorganization.platform_api/whoami_urls.py+ a mount inbase_urls.py— its own module because the spec's urlconf selector can only pick out a mount declared with a dotted module path (the tenant and public mounts pass a list, sourlconf_name.__name__isNonefor them). Mounted ahead of the tenant urlconf so resolution is deterministic rather than relying on fall-through; organisation-scoped paths are rewritten before routing and none of those rewrites can producewhoami/.OrganizationMiddlewarematches^/api/(v[12])/unstract/(?P<org_id>[^/]+)/, so/api/v1/unstract/whoami/parsedwhoamias the organisation and rewrotepath_infoto/api/v1/unstract/→ 404, withrequest.organization_id = "whoami"making the org-match guard atcustom_auth_middleware.py:94403 every valid key. The whitelist escape hatch (ORGANIZATION_MIDDLEWARE_WHITELISTED_PATHS, previously[]) returns early without ever settingrequest.organization_id, which that same line reads by bare attribute access →AttributeError→ 500. So: the path is added to that list, and the whitelist branch now sets the attribute. The second half is a latent-bug fix that protects any future organisation-less path —test_whoami.pyfails with exactly thatAttributeErrorif it is removed.This is not
WHITELISTED_PATHS: that list skips authentication entirely, which for this endpoint would mean answering with someone else's organisation or none at all. A test asserts it stays out.platform_api/openapi_schema.py— mirrorsapi_v2/openapi_schema.py: a spec-only serializer that never builds a response, withpermissionsourced fromApiKeyPermission.choicesso a new tier cannot reach the API without reaching the spec.auth=[{"platformKey": []}]is mandatory, not decorative — an operation that omits it regresses tocookieAuth/basicAuth, because DRF's unset authentication default gets introspected as a decision.api/v1/unstract— the whole tenant mount — which, against astartswithover the union, let anAPI_DEPLOYMENT_PATH_PREFIXpointed anywhere under that mount pass the gate. Caught in review by @hari-kuriakose and @chandrasekharan-zipstack and fixed in0e9a23a37; an overridden prefix now fails the gate again, which is verified rather than asserted;SPEC_URLCONFSgains the new urlconf; and the two tests that looped over every operation asserting deployment-specific facts now pin those to the deployment operations and assert only genuinely universal ones (401/403/500, and that each operation names exactly one declared bearer scheme) globally. Declaring400/404on an operation that carries no body and names no resource would hand clients a dead branch — the same sintest_only_the_execution_endpoint_...already guards against.Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
The two middleware changes are the risk surface, and both are additive:
ORGANIZATION_MIDDLEWARE_WHITELISTED_PATHSgoes from[]to one regex matching only/api/v1/unstract/whoami/. Cloud and on-prem.append()to this list rather than reassigning it, so their existing entries are unaffected.request.organization_id = Nonein the whitelist branch can only turn anAttributeErrorinto the skip the earlyreturnalready intended. No path reached that branch before this PR, since the list was empty.The new URL mount sits ahead of the tenant urlconf but cannot shadow it:
OrganizationMiddlewarerewrites every organisation-scoped path to/api/v1/unstract/<rest>/before routing, and no rewrite yieldswhoami/. Verified by hand against a running server —/api/v1/unstract/acme/api/deployment/still returns 200 with the same key, and a key from another organisation still gets 403 there.Cloud and on-prem inherit both the mount and the setting with no change in
unstract-cloud:cloud_base_urls.pyandonprem_base_urls.pybothfrom .base_urls import urlpatternsand append.The regenerated spec is +160/−0 — the deployment operations are byte-identical, so nothing generated from it changes.
Database Migrations
None. No model changes.
Env Config
None.
ORGANIZATION_MIDDLEWARE_WHITELISTED_PATHSis a settings constant, not an env var, and is not intended to be overridden per installation.Relevant Docs
None in this repo.
unstract-docshas aPlatform API Keyspage that could gain awhoamientry once this lands; not blocking.Related Issues or PRs
unstract auth whoami,docstudio deployment ls); PR pending repo accessDependencies Versions
None added or changed.
Notes on Testing
46 passedin the two suites this touches (platform_api/,api_v2/tests/test_docstudio_spec.py), of which 10 + 3 subtests are new endpoint tests and 6 are new spec anchors. Full backend suite baselined against the unmodified parent commit: identical failure set, +16 passing.platform_api/tests/test_whoami.pygoes through the real URLconf and a real middleware chain, because everything that makes this endpoint work happens before the view. It covers: the organisation resolving from the key and not the URL (two organisations, one URL, two answers); every tier reading its own identity; missing/malformed/unknown/inactive keys; that the route actuallyresolve()s, so a 404 cannot masquerade as a passing 401; and that the path is not inWHITELISTED_PATHS.Also exercised by hand against a running server with two seeded organisations —
whoamianswers each key with its own organisation over the same URL, all four rejection paths return 401, and the organisation-scoped regression checks above pass.Screenshots
n/a — no user-facing surface.
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code
https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM