Skip to content

[Feature] : API ENDPOINTS PR 1: Foundation and scaffolding#1130

Open
pulk17 wants to merge 1 commit into
CCExtractor:masterfrom
pulk17:api-pr1-scaffolding
Open

[Feature] : API ENDPOINTS PR 1: Foundation and scaffolding#1130
pulk17 wants to merge 1 commit into
CCExtractor:masterfrom
pulk17:api-pr1-scaffolding

Conversation

@pulk17

@pulk17 pulk17 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

[Feature]

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

My familiarity with the project is as follows (check one):

  • I have never used the project.
  • I have used the project briefly.
  • I have used the project extensively, but have not contributed previously.
  • I am an active contributor to the project.

Overview

First PR of a 7-part stack that adds a token-authenticated JSON REST API (/api/v1) to the sample platform, giving CI consumers and future tooling programmatic access to runs, samples, results, and logs — everything that today requires scraping the HTML views.

Stack: #1130#1131#1132#1133#1134#1135#1141. Merge bottom-up. Each PR contains the ones below it, so the diff shrinks automatically as parents merge.

This PR deliberately contains no endpoints. It lays down the infrastructure the six route PRs build on, so their reviews can focus on endpoint logic.

What's in it

Blueprint + middleware (mod_api/)

  • mod_api blueprint registered at /api/v1. Anything under that prefix returns structured JSON errors ({code, message, details}) — including routing-level 404/405s, which are converted app-wide via an after_app_request hook so no HTML error page ever leaks into an API response.
  • Bearer auth: Authorization: Bearer spci_... (scheme matched case-insensitively per RFC 7235). Tokens are 256-bit random secrets; the server stores only a SHA-256 hash plus an indexed 16-char prefix for lookup. Verification fetches candidates by prefix and compares hashes with hmac.compare_digest (constant-time). Scope checks (@require_scope) and role checks (@require_roles) are separate decorators, so each endpoint's requirements are visible at its definition.
  • Rate limiting: per-client sliding window with standard X-RateLimit-* response headers. In-memory by design — single-process deployment; the store interface is small enough to swap for Redis if that ever changes.
  • Security headers on every API response; hook registration order (and Flask's reverse execution order for after_request) is documented in mod_api/__init__.py.
  • Validation helpers: offset/cursor pagination, path-id, date-range, and sort-parameter validators used consistently by all later PRs.

Model + migration

  • ApiToken model: name (unique per user, kept after revocation for audit), hash, prefix, JSON scopes, expiry, revocation timestamps.
  • Alembic migration d4f8e2a1b3c7: new api_token table + nullable user.github_login column. Purely additive — no existing table is modified, so it's safe for the deployment pipeline to auto-apply, and rolling back the code without rolling back the schema is harmless.

Status derivation service (mod_api/services/status.py)
The single source of truth mapping raw TestProgress/TestResult/TestResultFile rows to normalized statuses (runs: queued/running/pass/fail/canceled/error/incomplete; samples: pass/fail/skipped/missing_output/running/not_started). Route handlers are forbidden from inlining their own derivation — this is what keeps /summary, /samples, and /errors from ever disagreeing. Documented data-model traps it encodes: TestResultFile.got = NULL means match, not missing; the (-1, 'error') dummy row means the test produced no output; test.failed only reflects cancellation. A run marked completed that produced zero result rows reports error, never pass.

Test scaffolding
tests/api/base.py provides ApiTestCase, which stubs the deliberately-slow sha512_crypt password hashing for API tests (they create users/tokens constantly). This keeps the API suite in the seconds range instead of minutes; any test needing real hashing can stop the patchers locally.

Testing

42 tests: token model (generation, hashing, prefix extraction, expiry), status derivation (including the completed-without-results → error case), validation helpers, and utils. Full lint battery (isort, pycodestyle, pydocstyle, dodgy, mypy) is green.

@cfsmp3 cfsmp3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the two high complexity errors (validation.py and status.py), also see Claude review below.

Reading the rview, clearly H1 must be addressed before we can merge or things will break.

The rest could be addressed (possibly are) later on in the stack.

But let's make sure we can merge and test each PR in order but not at the same time, i.e. merging this diff shouldn't break the system, even if by itself it doesn't do anything useful (since it's scaffolding).

Claude review follows:

HIGH (blocker):

  • H1 — ApiToken model added with NO migration. The api_token table won't exist on real MySQL. Tests pass only because tests/base.py does create_all from models → masks the gap. PR2's auth endpoints break at runtime. #1117 had this migration (d4f8e2a1b3c7); dropped in the split. Must add it, chained off master head c8f3a2b1d4e5.

MEDIUM (carryover, code unchanged):

  • ~770 lines of security middleware merge untested — and with no routes yet, the before_request hooks never even fire in this PR. Deferred to PR2/3. Risk noted.
  • Rate limiter unbounded memory (no hard cap, eviction only every 100 req).
  • Auth timing oracle (no-candidate path skips argon2 verify → leaks whether a prefix exists).
  • _get_client_ip comment wrong (ProxyFix means remote_addr is from XFF).

LOW/NIT: run-level missing→fail path untested; stale is_dummy_row "DEPLOYMENT PREREQUISITE" docstring; N+1 footgun in the batch_get_run_data wrappers; pytest conftest.py inert under nose2; generic 429 handler hardcodes wrong limits.

@pulk17
pulk17 force-pushed the api-pr1-scaffolding branch 9 times, most recently from 9950853 to beb4fe9 Compare June 25, 2026 10:25
@cfsmp3

cfsmp3 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

New / still open:

  • 🔴 N1 (Medium) — the timing-attack fix doesn't work. They added a dummy argon2 verify on the no-candidate path, but the hardcoded hash is malformed — I measured it: verify fails-fast in 0.0 ms vs ~43 ms for a real verify. So the timing oracle is still open, now hidden behind code that looks like a fix. Cheap fix: _DUMMY_HASH = PasswordHasher().hash('dummy') at import. (Practical risk is low — prefixes are 65-bit — but don't ship dead security code.)
  • 🟡 N2 (Medium) — migration/model drift: the migration adds user.github_login but mod_auth/models.py doesn't declare it → tests (create_all) won't have the column, and a future flask db migrate would propose dropping it. Add the field to the User model here, or defer that migration line to [Feature] : API ENDPOINTS PR 3 : System Status and Run Management Endpoints #1132 where it's used.
  • 🟡 M2 (rate-limit memory cap) still open.

@pulk17
pulk17 force-pushed the api-pr1-scaffolding branch 2 times, most recently from b9a645a to d1e2392 Compare June 26, 2026 19:33
@pulk17
pulk17 force-pushed the api-pr1-scaffolding branch from d1e2392 to c69c72d Compare July 20, 2026 10:22
@sonarqubecloud

Copy link
Copy Markdown

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