Production hardening: security, stability, maintainability (closes R4/R5/R6) - #53
Merged
Conversation
- Remove module-level filesystem side effects and hardcoded absolute paths - Skip automatically when the local doc corpus / OPENAI_API_KEY is absent - Unblocks pytest collection (was 1 hard collection error) Co-Authored-By: Claude <noreply@anthropic.com>
The index was declared twice (index=True on the column AND an explicit Index in __table_args__), causing 'index already exists' on create_all and breaking 85 repository tests. Keep the explicit Index, matching the email/user_id convention. Co-Authored-By: Claude <noreply@anthropic.com>
Addresses CRITICAL/HIGH/MEDIUM findings on the dashboard auth path: - Verify issuer (clerk_issuer) and authorized party (azp allowlist); reject tokens bound to an unexpected frontend origin - Assert JWK alg=RS256 and use=sig; refresh JWKS once on kid miss (key rotation no longer causes an outage) - Require exp/iat/sub claims - Account identity is the verified sub, never a client email; unverified emails are never stored/linked (squat-proof, enforced in the data layer) - Race-safe first-login via IntegrityError handling; 409 on email conflict - Add get_or_create_by_clerk_id repository method + 13 security tests Co-Authored-By: Claude <noreply@anthropic.com>
Second-round security-review hardening of the dashboard auth path: - Reject tokens with a missing azp when an allowlist is configured (was fail-open on absent azp) - Make issuer enforcement mandatory and fail closed (500) when clerk_issuer is unset; require the iss claim - Restrict verified-email account linking to DORMANT accounts only (no API keys, no Stripe) to prevent active-account takeover; 409 otherwise - Add tests for active-account and api-key-holding-account protection Co-Authored-By: Claude <noreply@anthropic.com>
Fixes surfaced by getting the test suite to green:
- yaml_frontmatter regex anchored with \A — was matching body-level '---'
horizontal rules under MULTILINE and devouring content between them
- whitespace normalization collapses blank-line runs to a single blank line
(no more 3+ consecutive newlines)
- no-API-key LLM validation now runs real fallback structure checks instead
of rubber-stamping is_valid=True; counts requests; lowered fallback content
threshold to a sane heuristic
- add navigation_link_row pattern ('Navigation:'-prefixed link rows)
- CRITICAL isolation fix: each cleaner gets its own deep-copied PatternRegistry
instead of sharing the mutable DEFAULT_REGISTRY singleton — prevents
pattern.enabled mutations leaking across instances / concurrent requests
- fix temp-dir fixture teardown (shutil.rmtree); refresh stale pattern counts
Co-Authored-By: Claude <noreply@anthropic.com>
…ity) - Fix real bug: processor checkpoint used processed_count before assignment (NameError mid-batch) -> use len(self.processed_docs) - Fix real bug: verify_env imported 'beautifulsoup4' (always failed); rewrite with importlib.util.find_spec and correct module names - Replace bare except (E722) with except Exception - Auto-fix unused imports / empty f-strings (66); prefix intentionally-unused locals with _ (preserves GUI app refs and side-effecting test calls) - Rename ambiguous loop var l -> line (E741) Co-Authored-By: Claude <noreply@anthropic.com>
- Add [tool.ruff], [tool.mypy], [tool.pytest.ini_options] to pyproject: pinned target-version, ignore_missing_imports + import-untyped, api.* held to a stricter bar, docscraper.* gradual (legacy), asyncio config - Real safety fix: scrape/process workers now guard against null job.url before scraping / building Paths (avoided runtime NameError/TypeError) - Type job result dict as dict[str, Any]; coerce DB strings to JobStatus/ BillingTier enums in responses; annotate billing._stripe; type: ignore the slowapi handler signature friction - mypy now clean on api/ (36 files) and src/ (19 files) Co-Authored-By: Claude <noreply@anthropic.com>
- ADR-008 documents the API-key (programmatic) + Clerk JWT (dashboard) model, superseding the single-auth assumption of ADR-004 - Update quality metrics to the post-hardening state (tests/ruff/mypy green) Co-Authored-By: Claude <noreply@anthropic.com>
alembic upgrade failed with ModuleNotFoundError: No module named 'api' (pre-existing: the repo root was never on sys.path for the alembic CLI). Adding prepend_sys_path = . fixes 'alembic upgrade head' in CI and deploy. Co-Authored-By: Claude <noreply@anthropic.com>
The initial migration (revision 69c02554) was silently ignored because the
broad '*api_key*' secret-guard in .gitignore matched its filename
('...users_api_keys...py'). CI therefore only had the clerk migration whose
down_revision pointed at the missing 69c02554 -> KeyError building the
revision map.
- Add '!**/*.py' negation so secret-name guards never swallow source/migrations
- Commit the initial schema migration so the chain is complete in CI/deploy
Co-Authored-By: Claude <noreply@anthropic.com>
CI test collection failed with ModuleNotFoundError: No module named 'api' because the top-level api package is not pip-installed and pytest's import mode did not add the repo root. pythonpath = ["."] makes it deterministic. Co-Authored-By: Claude <noreply@anthropic.com>
pytest's pythonpath ini was applied too late (tests/conftest.py imports api during early collection). A root-level conftest.py is loaded first and inserts the repo root, so 'import api' works regardless of pytest version. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Drives Parsify to a production-ready bar across security, stability, maintainability, and architecture. All changes verified green before integration.
kidasserted, issuer +azpverified (fail-closed), verified-email-only + squat-proof identity, dormant-only account linking, race-safe first login — 15 dedicated testsNameError, and worker null-safety crashesverify_envalways-failing import, processor counter)Closes R4, R5, R6.
Commits (8)
fix(api): harden Clerk JWT authentication (security)fix(api): close auth-review follow-ups (azp/issuer/account-linking)fix(db): remove duplicate clerk_user_id index definitionfix(core): correct cleaning-engine bugs and isolation (concurrency)fix(core): resolve all 90 ruff findings + 2 latent bugschore(config): add ruff/mypy/pytest config and fix api type-safetytest(core): make manual integration test import-safe and CI-skippabledocs(architecture): add ADR-008 dual-auth model; mark R4/R5/R6 closedTest/quality gate
pytest: 570 passed, 3 skipped, 0 errorsruff check .: All checks passedmypy api/+mypy src/: cleanDeployment note
New production config required for the dashboard auth path:
PARSIFY_CLERK_JWKS_URL,PARSIFY_CLERK_ISSUER,PARSIFY_CLERK_AUTHORIZED_PARTIES. Auth fails closed if the issuer is unset.🤖 Generated with Claude Code