Skip to content

Prep for #112 and #113: security review pack, OIDC https tests and real-IdP check - #229

Merged
amar-python merged 9 commits into
mainfrom
claude/issue-112-113-security-oidc-prep
Nov 10, 2026
Merged

amar-python merged 9 commits into
mainfrom
claude/issue-112-113-security-oidc-prep

Conversation

@amar-python

@amar-python amar-python commented Nov 10, 2026 •

Copy link
Copy Markdown
Owner

What

Preparation for the two trackers that can't be closed from code: #112 needs an independent reviewer, and #113 needs a person with access to the real identity provider. This PR makes both as cheap as possible. Neither tracker is closed by it.

Stacked on #228 (M7), on #227 (M6), on #226 (M5). The base is the M7 branch, so this diff shows only this work.

#113: OIDC against the real IdP

  • The https:// key fetch now has tests. The tracker said it ran in no test. tests/system/test_oidc_https.py runs PyJWT's PyJWKClient for real against a local TLS server (tests/oidc_stub.py): a trusted certificate, key caching, a forged token naming a published key, claim checks, key rotation, an untrusted certificate, an IdP that is down, one that hangs, and a full sign-in through the API.
  • The key fetch now times out after 5 s. PyJWT's default is 30 s, which would hold a request thread that long on every sign-in attempt while the IdP is unreachable. The hang test fails without the change.
  • A correction to a comment, found by writing the rotation test: api/oidc.py implied a rotated key is re-fetched immediately. PyJWT re-fetches at most every 30 s (so random key ids can't hammer the IdP), which means a freshly rotated key can be refused for up to 30 s. The comment, the env var doc and the runbook now say so, and a test pins both halves.
  • scripts/check_oidc.py + docs/OIDC_REAL_IDP_CHECK.md: the real check as one command. It runs config, discovery, the key set, the token and (with --api) the sign-in as separate stages, each with a hint for its failure, and never prints the ID token or the session token. The token comes from an environment variable, a file or stdin, never the command line. The runbook uses the device-code flow (Keycloak and Entra ID), so the token never touches a browser address bar or a third-party debugger.

#112: security review pack

docs/SECURITY_REVIEW_PACK.md maps the five areas in scope (authentication, tenant isolation, role matrix, upload handling, deployment secrets) to their controls, the code and the tests behind them, and questions for the reviewer, and records 11 weaknesses I found while preparing it, each with how it was verified. The headline ones:

F1 No limit on the request body before it is parsed, so a signed-in uploader can make the server buffer any size before the 413
F3 Rate limiting keys on the peer IP, so behind a reverse proxy everyone shares one budget
F7 One runtime role reaches every tenant's schema; the per-tenant roles in the original design were never built
F8 The shared API key bypasses tenants and roles, and every action it takes audits as api_key
F2 The unauthenticated /api/health returns the database name, host and port and exact PostgreSQL version; /docs and /openapi.json are open (verified by calling it)
F9 Key Vault and the registry are left publicly reachable "temporarily" until someone flips them; the API's own secrets have no deployment definition

None of these is fixed here. Whether to fix, accept or defer each is the review's output, and several (F3, F5, F7, F8) are product decisions.

  • make security-evidence runs the tests behind the pack: 231 pass.
  • The frontend's print window is the one place HTML is built by hand (same origin as the session token) and had no test. export.test.ts now checks that every user-supplied value is escaped there; breaking the escaping fails it.
  • pip-audit (Python) and npm audit --omit=dev (frontend) found nothing on 2026-11-10.

Decisions to check

  • The accessibility job now also runs the frontend's vitest suite (17 files, 171 tests). No job ran it before. It passes locally with the same Node 22 and npm ci. The step is separable: drop it from quality-gate.yml if you'd rather add it yourselves. (I first made it a separate job, but tests/unit/test_ci_quality_gate.py requires every job to have a Python setup and an artifact upload step, so it lives in the job that already has Node and npm ci.)
  • The pack is not a substitute for the review. It does not cover a penetration test, the live state of Key Vault and the Terraform state account (I didn't check them), a secret scan of git history, or TLS and proxy configuration. Section 9 lists these.
  • Severities in the pack are my estimates, for the reviewer to overrule.

What's still needed to close the trackers

Verification

Full suite for the M7 base passes (584 tests); this branch adds 29 Python tests (test_oidc_https.py 12, test_check_oidc.py 17) plus 3 frontend tests, and all 38 OIDC tests pass together. scripts/lint.sh clean, bandit shows only the existing api/db.py notes, docs/TEST_CASES.md regenerated.

https://claude.ai/code/session_011T6WqhUfEMZv2Dxerqs2Eq


Generated by Claude Code

…113)

#95 tested OIDC with a local key and a file:// key set, so the https:// path
(PyJWT's PyJWKClient) ran in no test. These run it for real against a local TLS
identity provider: a trusted certificate, key rotation, an untrusted certificate,
an IdP that is down, one that hangs, and a full sign-in through the API.

- The key fetch now times out after 5 s. PyJWT's default is 30 s, which would hold
  a request thread that long for every sign-in attempt while the IdP is unreachable.
- oidc.verify() keeps the reason a token is rejected; username_from() still hides it.
- Corrects a comment: a rotated key is re-fetched at most every 30 s (PyJWT's
  cooldown), so it can be refused for up to 30 s after rotation. Tested.
- scripts/check_oidc.py runs the check against the real IdP, stage by stage, with a
  hint for each failure and no token or session token ever printed.
  docs/OIDC_REAL_IDP_CHECK.md is the procedure (device-code flow, Keycloak and Entra).

One sign-in against the deployment's real IdP is still needed to close #113; this
makes it a single command.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6WqhUfEMZv2Dxerqs2Eq
…ts evidence

Preparation for the independent review ADR-0003 asks for before the first external
tenant; not the review. docs/SECURITY_REVIEW_PACK.md maps the five scoped areas
(authentication, tenant isolation, role matrix, upload handling, deployment secrets)
to their controls, code and tests, lists the questions for the reviewer, and records
11 weaknesses found while preparing it (F1 to F11), each with how it was verified.

- make security-evidence runs the tests behind it (231 pass).
- The print window in the frontend is the one hand-built HTML sink and had no test;
  export.test.ts now checks that every user-supplied value is escaped there.
- A CI job runs the frontend vitest suite, which no job ran before.
- pip-audit and npm audit (production) were clean on 2026-11-10.

Nothing found has been fixed: whether to fix, accept or defer each is the review's output.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6WqhUfEMZv2Dxerqs2Eq
)

tests/unit/test_ci_quality_gate.py requires every job to have a Python setup and
an artifact upload step, which a Node-only job doesn't. The accessibility job
already has Node, npm ci, Python and the upload, so the vitest step goes there.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6WqhUfEMZv2Dxerqs2Eq

Copy link
Copy Markdown
Owner Author

The accessibility (Playwright + axe-core) check failed on the previous head with the same scenario and violation as on #226 and #228: upload_mapping_dialog, a color-contrast result on 13 muted-text elements. It isn't caused by this PR (no frontend files change here; the frontend tests in the same job passed, 171 of 171).

A fix for it already exists: 715469c on #226's branch makes the scan wait for running animations to finish, because a dialog still fading in is blended with the page behind it. That branch hadn't reached this one, so I've merged it in (via the M7 branch) rather than leave this PR red until #226 merges. This push re-runs the checks.


Generated by Claude Code

@amar-python
amar-python marked this pull request as ready for review November 10, 2026 05:16
Base automatically changed from claude/issue-111-sovereignty-reports to main November 10, 2026 05:28
@amar-python
amar-python merged commit 91cffef into main Nov 10, 2026
4 checks passed
@amar-python
amar-python deleted the claude/issue-112-113-security-oidc-prep branch November 10, 2026 05:29
amar-python pushed a commit that referenced this pull request Nov 10, 2026
The Streamlit exclude test waited for "\d+ excluded", which "0 excluded"
already matches, so it clicked the grid mid-redraw. It now waits for a
non-zero count and for the dialog to close, the grid click retries, and
the suite's expect timeout is 10 s for cold starts.

docs/ISSUE_TESTS.md: #109, #110, #111, #112, #113, #172, #184 and #195 now
point at the tests that merged; #175 is guarded by outcome since M5.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6Wqi8yaiaYWUVH8C4QAY
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.

2 participants