Skip to content

Repository hardening: CI, security scanning, and governance#8

Merged
AdamEXu merged 8 commits into
devfrom
security/repo-hardening
Jul 8, 2026
Merged

Repository hardening: CI, security scanning, and governance#8
AdamEXu merged 8 commits into
devfrom
security/repo-hardening

Conversation

@AdamEXu

@AdamEXu AdamEXu commented Jul 7, 2026

Copy link
Copy Markdown
Member

Repository hardening

This repo had zero repository automation — no .github/, no CI, no branch protection, no secret scanning, no Dependabot, no LICENSE, and no private path to report a vulnerability — while being public and handling live student data (Google + Schoology OAuth tokens, a Fernet encryption key). This PR adds the automation and governance layer. Built via a multi-agent workflow that authored and verified every file (typecheck, ruff, pytest, YAML parse all run locally).

What's in here

CI (.github/workflows/)

  • ci-frontendtypecheck (tsc --noEmit) is the hard gate (verified green). lint and build run non-blocking for now (see caveats).
  • backendruff check + python -m compileall (byte-compile). Green.

Security scanning

  • codeql.yml — CodeQL for JavaScript/TypeScript and Python (security-extended), the exact bug classes being firefought (SSRF, IDOR, injection).
  • bandit.yml — Python SAST, results to the Security tab.
  • dependabot.yml — weekly pnpm (/frontend), pip (/backend), and github-actions updates.

Governance / community health

  • LICENSE (MIT) — the repo was legally un-reusable without one.
  • SECURITY.md — private vulnerability reporting via GitHub advisories (no public issue for vulns).
  • pull_request_template.md — with an IDOR / SSRF / authz / secrets / token-encryption checklist drawn from the recent fix/* history.
  • CODEOWNERS, issue templates, CONTRIBUTING.md (wired to the real Makefile targets).

Quality tooling

  • backend/ruff.toml (F,I select), requirements-dev.txt, root .pre-commit-config.yaml (ruff + detect-private-key + large-file guard).
  • First tests in the repobackend/tests/ covers the security-sensitive functions: encryption fail-closed, @auth_required, JWT audience/issuer/exp + alg=none rejection. 17 tests, all passing.

Fixes made so CI is green on arrival

  • frontend: pnpm lint was hard-crashing (FlatCompat × react-hooks@7 circular JSON) → migrated to eslint-config-next native flat config.
  • backend: applied ruff safe autofixes (import sorting, ~30 files) + fixed a real runtime NameError (Any used without import in services/chat/service.py) and dropped two unused assignments.

⚠️ Concerns surfaced by the audit — please review

  1. Frontend has ~71 pre-existing eslint errors (no-explicit-any, <a>-instead-of-Link, react-hooks set-state-in-effect, etc.). Out of scope here; lint is non-blocking until burned down. Flip it to a hard gate afterward.
  2. next build not verifiable in CI yet — it needs NEXT_PUBLIC_* env (Convex, PostHog) as repo secrets/vars. build is non-blocking until those are wired; then make it a hard gate.
  3. Latent bug (not caught by current ruff select): services/chat/service.py ~370–406 has B023 — closures in a loop capturing the loop variable (late-binding footgun). Worth a look; enable ruff B rules later to gate it.
  4. No SSRF/URL-allowlist validator found in backend/services/scraper (google_drive.py, attachment_download.py). Given the fix/scraper-attachment-ssrf branch, confirm outbound fetches can't be driven by user-controlled URLs. No SSRF test written because there's no validator to test.
  5. ruff format --check (would reformat ~46 files) and the churnier ruff UP/B rules are deliberately deferred to their own follow-up commit.

✅ Secret-scan result (the "anything concerning" check)

Full git-history scan (gitleaks, 67 commits) + path/pattern sweep: no leaked credentials. No .env, key, .pem, or .db file was ever committed. Nothing to rotate.

Repo settings (not part of this PR — applied/queued separately)

Branch protection + required status checks (frontend, backend, CodeQL) should be enabled after this PR's checks run green once so they reference real check names. Secret scanning, push protection, Dependabot alerts, and private vulnerability reporting are being enabled via repo settings.

🤖 Generated with Claude Code

AdamEXu and others added 4 commits July 7, 2026 13:43
Adds the repository automation this public repo was missing entirely:

- CI: ci-frontend (typecheck hard gate; lint/build non-blocking ratchets)
  and backend (ruff check + byte-compile) on PRs to main
- Security scanning: CodeQL (JS/TS + Python, security-extended) and
  bandit SAST (SARIF -> Security tab)
- Dependabot: weekly pnpm (/frontend), pip (/backend), and github-actions
  updates, grouped minor+patch
- Governance: LICENSE (MIT), SECURITY.md with private-advisory reporting,
  PR template with an IDOR/SSRF/authz security checklist, CODEOWNERS,
  issue templates, CONTRIBUTING.md
- Tooling config: backend/ruff.toml (F,I select), requirements-dev.txt,
  root .pre-commit-config.yaml (ruff + detect-private-key + large-file guard)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`pnpm lint` was hard-crashing (TypeError: Converting circular structure
to JSON) because eslint.config.mjs pulled next/core-web-vitals via the
legacy FlatCompat shim, which chokes on eslint-plugin-react-hooks@7's
circular plugin object. Switch to eslint-config-next's native flat
exports so eslint runs again.

Also add a `typecheck` script (tsc --noEmit) — the CI hard gate — and
pin packageManager to pnpm@10.30.3 plus a .nvmrc (node 22).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…igns

Applies ruff's safe autofixes (import sorting/pruning across ~30 files)
so the new `ruff check` CI gate is green, and fixes the non-autofixable
findings it surfaced:

- services/chat/service.py: `list[dict[str, Any]]` used with no import
  of `Any` (F821) — a latent runtime NameError. Add `from typing import Any`.
- api/routes.py: drop two unused `result =` assignments (kept the calls).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
First tests in the repo, targeting the security-sensitive pure functions
behind the recent fix/* branches:

- test_encryption.py: token encrypt/decrypt round-trip; fails closed on
  tampered/garbage ciphertext
- test_middleware.py: @auth_required returns 401 without a valid session,
  injects the user with one
- test_jwt.py: audience/issuer/exp enforcement; rejects alg=none and
  tampered signatures

conftest.py sets a generated Fernet key + required env before import so
runs are hermetic. 17 tests, all passing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-advanced-security

Copy link
Copy Markdown

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

@github-advanced-security github-advanced-security AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bandit found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

CodeQL flagged a log-injection sink: request.args was logged verbatim at
DEBUG, letting an attacker forge log lines via CR/LF and leaking the OAuth
code/state into logs. Log only sanitized key names instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@AdamEXu

AdamEXu commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

CodeQL triage — the scanner immediately caught 4 pre-existing alerts

Enabling CodeQL paid off on day one. Triaged read-only against the actual data flows:

# Alert Location Verdict
1a SHA256 "weak hash on password" (HIGH) db/tokens.py:13 False positive — hashes a high-entropy, server-generated Schoology OAuth request token used as an O(1) DB lookup key, not a user password. Bare SHA256 is defensible here; a slow KDF would break the lookup. Optional: switch to HMAC-SHA256 for consistency.
1b SHA256 "weak hash on password" (HIGH) services/mobile/service.py:76 False positive — already hmac.new(secret, token, sha256) over secrets.token_urlsafe(...). Best practice; no change.
2 Log injection (MED) auth/routes.py:51 Genuine (low sev) — fixed in 7fdfcb9. Raw request.args was logged verbatim at DEBUG (CR/LF log forging + leaked OAuth code/state); now logs only sanitized key names.
3 Open redirect (MED) mobile/routes.py:293 False positive — gated by is_valid_bootstrap_redirect(), a strict same-origin + /mobile/onboarding path-prefix allowlist. CodeQL doesn't recognize the sanitizer.

Net: 1 real low-severity issue (fixed), 3 false positives. No genuine high-severity vulnerability.

The CodeQL PR check will stay red until alerts 1a/1b/3 are dismissed as false-positive (Security → Code scanning → Dismiss → "Used in tests / won't fix"). Optional hardening if you'd rather resolve than dismiss: make db/tokens.py use HMAC-SHA256 (clears 1a), and reconstruct the redirect from the validated path in mobile/routes.py (clears 3). Happy to do either — just say which.

Comment thread backend/auth/routes.py
"""Handle Google OAuth callback"""
logger.debug("Google OAuth callback hit")
logger.debug(f"Request args: {request.args}")
logger.debug("Request args keys: %s", sorted(request.args.keys()))
AdamEXu and others added 2 commits July 7, 2026 14:03
…cryption tests, drop superseded test_jwt.py

The baseline suite (PR #7) and this branch both added conftest.py and
test_encryption.py. The baseline versions are strictly stronger (idempotent
secret provisioning, MAIN/SESSIONS_DB_PATH sentinel guards, env-var key
binding), so they win. test_jwt.py is superseded by the baseline's
test_jwt_utils.py, which covers the same surface without the wall-clock
sleep; test_middleware.py is unique to this branch and stays.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The backend workflow lint+compile gates never executed the tests this PR
ships. Install requirements.txt + requirements-test.txt and run pytest as
a hard gate, and also trigger on pushes to dev (the integration branch).
Import-sort the baseline test files so the merged tree passes its own
ruff gate.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AdamEXu AdamEXu changed the base branch from main to dev July 8, 2026 00:24
# Conflicts:
#	backend/api/routes.py
#	backend/services/schoology/client.py
#	backend/tests/test_mobile_tokens.py
#	backend/tests/test_scraper_section_leases.py
@AdamEXu AdamEXu marked this pull request as ready for review July 8, 2026 00:30
@AdamEXu AdamEXu merged commit fe14a1b into dev Jul 8, 2026
1 check passed
AdamEXu added a commit that referenced this pull request Jul 8, 2026
Integrate the Convex removal with everything on dev (#1-#14). Resolutions:
- api/routes.py: keep #15's fail-closed SQLite ownership check (restores PR
  #4's IDOR guarantee against the new source of truth).
- services/schoology/client.py: graft PR #3's full SSRF hardening
  (validate_outbound_url + per-hop re-validating redirect loop) onto #15's
  rewritten client, which only had host-scoped auth.
- auth/routes.py: keep PR #8's log-injection fix (log arg keys, not values).
- Convex modules/frontend deleted by #15 (convex_sync, internal_chat,
  convex/, ConvexClientProvider): keep deleted.
- chat/page.tsx: take #15's SSE rewrite, which already re-implements PR #5's
  input-preservation and loading-vs-not-entitled behavior. Tool-call
  rehydration on reconnect remains a known follow-up (needs an SSE-era
  endpoint).
- tests/test_jwt_utils.py: drop tests for the Convex JWT audience and JWKS
  document #15 removed; algorithm-confusion coverage retargeted to the
  mobile audience, not deleted.

Verified on the merged tree: ruff clean, 142 passed + 2 xpassed, smoke_stores
36/36, app imports clean, frontend tsc --noEmit clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AdamEXu added a commit that referenced this pull request Jul 8, 2026
These files predate the ruff config #8 added, so CI's ruff gate flagged them
once the branch was merged with dev. Pure formatting — import sorting and one
placeholder-less f-string; no logic change (compile + full pytest still green).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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