Chore/dependency security audit - #2
Conversation
requirements.txt declared pydantic twice: `pydantic==2.11.3` followed by a bare, unpinned `pydantic[email]`. The `safety` scanner flags the unpinned specifier because it nominally admits ancient pydantic 1.x releases carrying known CVEs. pip intersected the two constraints and resolved 2.11.3 regardless, so no vulnerable version was ever installed — but the range is now closed explicitly instead of depending on that resolver behaviour, and the duplicate declaration is gone. Verified: pip resolves pydantic 2.11.3 with the email extra; `safety check` goes from "0 reported, 4 ignored" to "0 reported, 0 ignored"; pip-audit remains clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Formatting-only change — no behaviour, no logic, no dependency changes. Black is AST-preserving; verified after the reformat: black --check . -> 56 files unchanged ruff check . -> all checks passed pytest -> 13/13 passed clean-room boot -> /health 200, protected 401, unknown 404 Kept as its own commit so the preceding security/MongoDB work stays reviewable. Add this commit to .git-blame-ignore-revs (done in the follow-up commit) so `git blame` skips it. black is pinned in requirements-dev.txt and enforced by a dedicated `black --check .` step in the python-lint CI job. Its line-length is set to 100 in pyproject.toml to match [tool.ruff], so the two tools cannot disagree about where to wrap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Points git blame past d621b5e (the formatting-only commit) so line history still attributes code to whoever actually wrote it. GitHub picks this file up automatically; locally, enable it with: git config blame.ignoreRevsFile .git-blame-ignore-revs Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds §12 to SECURITY_AUDIT.md covering this pass: - pydantic duplicate/unpinned declaration found by `safety` (pip-audit missed it) - the bare-`pytest` sys.path defect that would have failed the python-test job - black adoption as an isolated commit + .git-blame-ignore-revs - lock-file inventory: uv.lock is a stub that pins nothing; no poetry/Pipfile - rationale for cross-checking with `safety` manually rather than wiring it into CI (its modern `scan` command requires an API key; `check` is EOL) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d, and placement modules with input validation and security safeguards.
…, and integration testing infrastructure
… and services, and add database indexing scripts
… and validation tests
…rotation documentation
…sk policies and add load testing token utility
There was a problem hiding this comment.
🟡 Not ready to approve
There are a few concrete correctness/security gaps (notably ai-service rate limiting not actually enforced, and node-api→ai-service calls not failing fast when the shared secret is unset) plus some cleanup needed for unused/possibly-confusing dependency artifacts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR performs a backend security/dependency audit and restructures the backend into a single public Node API plus a narrow internal FastAPI AI microservice, while removing the legacy duplicate python-service/ stack.
Changes:
- Removes the legacy
python-service/FastAPI backend and updates top-level docs to reflect the new two-service architecture (node-api/+ internalai-service/). - Adds Redis-optional infrastructure in
node-api(cross-instance WS broadcast + Redis-backed rate limiting), new health/readiness/metrics endpoints, and multiple security hardening fixes (validation, error codes, PII minimization). - Migrates Groq-backed AI features out of
node-apiintoai-service, with JWT-authenticated server-to-server calls and regression/integration test coverage for the new behavior.
File summaries
| File | Description |
|---|---|
| REQUIREMENTS.md | Updates repo prerequisites and service layout documentation. |
| README.md | Documents python-service removal and introduces ai-service as internal-only. |
| python-service/uv.lock | Removes legacy python-service lockfile. |
| python-service/tests/test_security.py | Removes legacy python-service tests. |
| python-service/tests/test_routes.py | Removes legacy python-service tests. |
| python-service/tests/conftest.py | Removes legacy python-service test env setup. |
| python-service/seed_mongo.py | Removes legacy python-service seeding script. |
| python-service/requirements.txt | Removes legacy python-service dependency manifest. |
| python-service/REQUIREMENTS.md | Removes legacy python-service requirements doc. |
| python-service/requirements-dev.txt | Removes legacy python-service dev requirements. |
| python-service/README.md | Removes legacy python-service README. |
| python-service/pyproject.toml | Removes legacy python-service pyproject/ruff config. |
| python-service/Dockerfile | Removes legacy python-service container image definition. |
| python-service/app/services/init.py | Removes legacy python-service package stub. |
| python-service/app/schemas/user.py | Removes legacy python-service schemas. |
| python-service/app/schemas/placement.py | Removes legacy python-service schemas. |
| python-service/app/schemas/persistence.py | Removes legacy python-service schemas. |
| python-service/app/schemas/interview.py | Removes legacy python-service schemas. |
| python-service/app/schemas/common.py | Removes legacy python-service schemas. |
| python-service/app/schemas/college.py | Removes legacy python-service schemas. |
| python-service/app/schemas/batch.py | Removes legacy python-service schemas. |
| python-service/app/schemas/auth.py | Removes legacy python-service schemas. |
| python-service/app/schemas/assessment.py | Removes legacy python-service schemas. |
| python-service/app/schemas/achievement.py | Removes legacy python-service schemas. |
| python-service/app/schemas/init.py | Removes legacy python-service package stub. |
| python-service/app/repositories/user_repo.py | Removes legacy python-service repository code. |
| python-service/app/repositories/college_repo.py | Removes legacy python-service repository code. |
| python-service/app/repositories/base.py | Removes legacy python-service repository base. |
| python-service/app/repositories/init.py | Removes legacy python-service package stub. |
| python-service/app/mongodb.py | Removes legacy python-service Mongo client code. |
| python-service/app/mongodb_sync.py | Removes legacy python-service Mongo sync client code. |
| python-service/app/main.py | Removes legacy python-service app entrypoint. |
| python-service/app/dependencies.py | Removes legacy python-service DI helpers. |
| python-service/app/database.py | Removes legacy python-service DB helpers. |
| python-service/app/core/websocket_manager.py | Removes legacy python-service WS manager. |
| python-service/app/core/security.py | Removes legacy python-service JWT/security module. |
| python-service/app/core/middleware.py | Removes legacy python-service middleware. |
| python-service/app/core/exceptions.py | Removes legacy python-service exception hierarchy. |
| python-service/app/core/init.py | Removes legacy python-service package stub. |
| python-service/app/config.py | Removes legacy python-service settings module. |
| python-service/app/api/v1/tests.py | Removes legacy python-service routes. |
| python-service/app/api/v1/router.py | Removes legacy python-service router. |
| python-service/app/api/v1/placements.py | Removes legacy python-service routes. |
| python-service/app/api/v1/persistence.py | Removes legacy python-service routes. |
| python-service/app/api/v1/colleges.py | Removes legacy python-service routes. |
| python-service/app/api/v1/chat.py | Removes legacy python-service routes. |
| python-service/app/api/v1/batches.py | Removes legacy python-service routes. |
| python-service/app/api/v1/ai.py | Removes legacy python-service Groq AI routes. |
| python-service/app/api/v1/achievements.py | Removes legacy python-service routes. |
| python-service/app/api/v1/init.py | Removes legacy python-service package stub. |
| python-service/app/api/router.py | Removes legacy python-service top-level router. |
| python-service/app/api/init.py | Removes legacy python-service package stub. |
| python-service/app/init.py | Removes legacy python-service package stub. |
| python-service/.python-version | Removes legacy python-service Python version pin. |
| python-service/.env.example | Removes legacy python-service env example. |
| python-service/.dockerignore | Removes legacy python-service dockerignore. |
| package-lock.json | Adds a repo-root npm lockfile (currently without a root package.json). |
| node-api/src/websocket/redisBroadcaster.js | Adds Redis-backed WS message broadcasting for multi-instance deployments. |
| node-api/src/websocket/broadcaster.js | Introduces pluggable broadcaster interface with Redis/in-memory implementations. |
| node-api/src/validations/placement.validation.js | Accepts/normalizes frontend string payloads to arrays for consistent storage. |
| node-api/src/validations/auth.validation.js | Accepts snake_case auth fields from the live frontend and adds change-password validation. |
| node-api/src/utils/regex.js | Adds regex escaping/length bounding to prevent regex injection/ReDoS in Mongo $regex usage. |
| node-api/src/utils/groqClient.js | Removes direct Groq client wrapper (AI calls moved to ai-service). |
| node-api/src/utils/ApiResponse.js | Adds response timestamps, richer pagination metadata, and a compatibility wrapper. |
| node-api/src/utils/ApiError.js | Adds stable machine-readable error codes and helpers for consistent error contracts. |
| node-api/src/utils/aiServiceClient.js | Adds node-api → ai-service HTTP client with per-request JWT minting and timeouts. |
| node-api/src/services/user.service.js | Fixes authorization paths (admin vs self-service) and institution-admin approval workflow. |
| node-api/src/services/test.service.js | Migrates assessment question generation to ai-service with graceful local fallback when unreachable. |
| node-api/src/services/student.service.js | Prevents PII exposure on non-staff student listing and fixes institution-scoped roll-number lookup. |
| node-api/src/services/placement.service.js | Preserves array response bodies while surfacing true totals via headers for HR UX compatibility. |
| node-api/src/services/interview.service.js | Migrates interview question generation to ai-service with local fallback when unreachable. |
| node-api/src/services/faculty.service.js | Enforces institution scoping on faculty listing using actor-based scoping. |
| node-api/src/services/email.service.js | Minor doc clarification while keeping “optional at boot” behavior. |
| node-api/src/services/department.service.js | Uses actor-based institution scoping (Express 5 req.query immutability) and returns unpaginated lists. |
| node-api/src/services/collegeAdmin.service.js | Enforces institution scoping on college-admin listing using actor-based scoping. |
| node-api/src/services/BaseService.js | Adds server-side whitelisted sorting support for list endpoints. |
| node-api/src/services/auth.service.js | Adds authenticated password change that bumps token_version and returns fresh tokens. |
| node-api/src/server.js | Ensures graceful shutdown also closes broadcaster + Redis client. |
| node-api/src/routes/user.routes.js | Enables institution-admin approvals and allows self-service user updates (service-layer enforcement). |
| node-api/src/routes/profile.routes.js | Adds upload verification middleware step before profile persistence. |
| node-api/src/routes/index.js | Adds /jobs alias to placement routes for legacy frontend compatibility. |
| node-api/src/routes/health.routes.js | Adds /health/live, /health/ready, /health/metrics endpoints for ops. |
| node-api/src/routes/auth.routes.js | Adds authenticated /auth/change-password route. |
| node-api/src/repositories/student.repository.js | Makes roll-number lookups institution-scoped and adds safe/accurate search pagination metadata. |
| node-api/src/repositories/institution.repository.js | Escapes regex input for unauthenticated institution name lookups. |
| node-api/src/repositories/BaseRepository.js | Adds sort whitelisting/building and plumbs sort into findAll. |
| node-api/src/middlewares/validate.js | Uses stable VALIDATION_ERROR code for validation failures. |
| node-api/src/middlewares/scopeInstitution.js | Fixes Express 5 req.query mutation no-op and documents required service-level scoping. |
| node-api/src/middlewares/requestLogger.js | Adds structured request completion logs for monitoring/benchmarking. |
| node-api/src/middlewares/requestId.js | Adds request correlation IDs (accept inbound or generate). |
| node-api/src/middlewares/rateLimitStore.js | Adds Redis-optional hybrid store for express-rate-limit with self-healing fallback. |
| node-api/src/middlewares/rateLimiter.js | Switches rate limiting to the hybrid store and adds test/local override knobs. |
| node-api/src/middlewares/errorHandler.js | Standardizes error response shape (detail/code/timestamp) and maps common infra errors safely. |
| node-api/src/middlewares/authenticate.js | Avoids leaking JWT verification reasons (generic unauthorized). |
| node-api/src/controllers/user.controller.js | Passes actor into approve/reject for proper service-level authorization. |
| node-api/src/controllers/resumeBuilder.controller.js | Fixes staff listing by passing actor into service.list (prevents crash/leak). |
| node-api/src/controllers/placement.controller.js | Exposes true totals via X-Total-Count while keeping body shape stable. |
| node-api/src/controllers/leaderboard.controller.js | Uses compatibility response wrapper for legacy frontend response shape assumptions. |
| node-api/src/controllers/faculty.controller.js | Fixes list handler to pass actor for scoping. |
| node-api/src/controllers/department.controller.js | Fixes list handler to pass actor for scoping. |
| node-api/src/controllers/dashboard.controller.js | Uses compatibility response wrapper for legacy frontend response shape assumptions. |
| node-api/src/controllers/collegeAdmin.controller.js | Fixes list handler to pass actor for scoping. |
| node-api/src/controllers/auth.controller.js | Adds snake_case support, refresh compatibility payload, and authenticated change-password handler. |
| node-api/src/config/redis.js | Adds optional shared Redis client with bounded retries and lifecycle management. |
| node-api/src/config/env.js | Replaces groq config with ai-service configuration (url/shared secret/timeout). |
| node-api/src/config/database.js | Adds ping() for readiness checks. |
| node-api/src/app.js | Adds request ID + structured logging, and registers health endpoints before global rate limiting. |
| node-api/src/tests/integration/uploads.test.js | Adds regression tests for secure upload validation. |
| node-api/src/tests/integration/studentIdentify.test.js | Adds regression tests for institution-scoped roll-number identification. |
| node-api/src/tests/integration/resumeBuilder-staff-list.test.js | Adds regression tests ensuring staff listing doesn’t crash/leak across institutions. |
| node-api/src/tests/integration/pagination.test.js | Adds regression tests for pagination metadata and sort support. |
| node-api/src/tests/integration/hr-jobs.test.js | Adds regression tests for /jobs alias and frontend payload shape. |
| node-api/src/tests/integration/collegeAdmins.test.js | Adds regression tests for institution-scoped college-admin listing. |
| node-api/src/tests/integration/aiFeatures.test.js | Adds regression tests for AI feature graceful degradation when ai-service is unreachable. |
| node-api/src/tests/helpers/testApp.js | Adds shared in-memory MongoDB test harness and env bootstrapping. |
| node-api/src/tests/helpers/seed.js | Adds repository-level seed helpers for integration tests. |
| node-api/scripts/setupIndexes.js | Adds missing indexes (token hashes, question category) to reduce scans and improve security/perf. |
| node-api/scripts/mintLoadTestToken.js | Adds load-testing token mint helper to bypass auth limiter during benchmarks. |
| node-api/README.md | Adds ops/security docs references (Redis/operations/credential rotation). |
| node-api/package.json | Bumps Node/Express major and adds Redis deps + loadtest scripts. |
| node-api/jest.config.js | Adds Jest config to avoid helper discovery and stabilize integration test runtime. |
| node-api/eslint.config.js | Tightens no-unused-vars and declares Node globals used by new fetch/AbortController code. |
| node-api/docs/DEPLOYMENT.md | Documents Redis setup and health/readiness/metrics endpoints for deployments. |
| node-api/docs/API.md | Updates API contract docs (timestamps, codes, pagination metadata, health endpoints). |
| node-api/Dockerfile | Bumps runtime to Node 24 and fixes uploads directory permissions for non-root runs. |
| node-api/.nvmrc | Pins Node 24 for local dev. |
| node-api/.node-version | Pins Node 24 for local dev. |
| node-api/.env.node.example | Documents Redis + limiter overrides and ai-service shared secret config. |
| node-api/.dockerignore | Ensures .env is excluded from Docker build context. |
| deploy/nginx.conf | Adds local multi-replica entrypoint config to reproduce cross-instance behavior. |
| ai-service/tests/test_security.py | Adds tests for service JWT auth and “default secret forbidden in production” startup guard. |
| ai-service/tests/test_interview.py | Adds tests for interview generation fallback and Groq-mocked path. |
| ai-service/tests/test_health.py | Adds tests for health/readiness endpoints without auth. |
| ai-service/tests/test_assessment.py | Adds tests for assessment question generation fallback and Groq error mapping. |
| ai-service/tests/conftest.py | Adds shared FastAPI TestClient + env overrides/fixtures. |
| ai-service/tests/init.py | Adds tests package marker. |
| ai-service/requirements.txt | Adds ai-service dependency manifest (loose ranges). |
| ai-service/requirements-lock.txt | Adds ai-service locked dependency set for reproducible Docker installs. |
| ai-service/requirements-dev.txt | Adds ai-service dev/test/audit tooling requirements. |
| ai-service/README.md | Documents ai-service purpose, auth model, fallbacks, and dependency locking approach. |
| ai-service/pytest.ini | Adds pytest config for asyncio + pythonpath. |
| ai-service/Dockerfile | Adds ai-service container image using requirements-lock.txt. |
| ai-service/app/security.py | Implements HS256 JWT verification for node-api → ai-service internal calls. |
| ai-service/app/schemas/resume.py | Adds request/response schemas for resume AI endpoints. |
| ai-service/app/schemas/interview.py | Adds request/response schemas for interview generation endpoint. |
| ai-service/app/schemas/assessment.py | Adds request/response schemas for assessment question generation. |
| ai-service/app/schemas/init.py | Adds schemas package marker. |
| ai-service/app/routers/interview.py | Implements interview generation endpoint with Groq path + fallback pool. |
| ai-service/app/routers/assessment.py | Implements assessment question generation endpoint with Groq path + fallback. |
| ai-service/app/routers/init.py | Adds routers package marker. |
| ai-service/app/main.py | Adds FastAPI app, startup guard for default secret, and health/readiness endpoints. |
| ai-service/app/groq_client.py | Adds Groq HTTP client wrapper with structured error handling. |
| ai-service/app/db.py | Adds optional Mongo logging for observability (best-effort, non-blocking). |
| ai-service/app/config.py | Adds typed settings with safe defaults and “production requires non-default secret” semantics. |
| ai-service/app/init.py | Adds app package marker. |
| ai-service/.env.example | Adds ai-service environment template. |
| .gitignore | Updates ignores for pytest/ruff caches and removes python-service uploads ignore. |
| .git-blame-ignore-revs | Adds ignore-revs file for formatting-only commits. |
| .env.compose.example | Adds docker-compose env template including shared secret + service URLs. |
Review details
Files not reviewed (1)
- node-api/package-lock.json: Generated file
Suppressed comments (1)
ai-service/app/main.py:43
- SlowAPI default limits won’t be applied without SlowAPIMiddleware. Right now only the exception handler is registered, so requests won’t be rate-limited.
app = FastAPI(title="UpScaler-AI — AI Service", version="1.0.0")
app.state.limiter = limiter
app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)
- Files reviewed: 183/191 changed files
- Comments generated: 5
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| // Shared ioredis client, used by both the rate limiter store | ||
| // (middlewares/rateLimitStore.js) and the WebSocket broadcaster | ||
| // (websocket/redisBroadcaster.js) — one connection, one place that owns its | ||
| // lifecycle and reconnection behavior, rather than each feature managing its | ||
| // own client. |
| "ioredis": "^6.0.0", | ||
| "joi": "^17.13.0", | ||
| "jsonwebtoken": "^9.0.2", | ||
| "mongodb": "^6.10.0", | ||
| "morgan": "^1.10.0", | ||
| "multer": "^2.0.0", | ||
| "rate-limit-redis": "^4.3.1", | ||
| "winston": "^3.13.0", |
| async function callAiService(path, body) { | ||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), env.aiService.timeoutMs); | ||
|
|
| motor>=3.6 | ||
| pymongo>=4.10 | ||
| python-jose[cryptography]>=3.3 | ||
| slowapi>=0.1.9 | ||
| httpx>=0.27 |
|
@copilot please fix the merge conflicts in this pull request. |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Resolved the merge conflict and pushed the fix in commit |
| router.get('/ready', async (req, res) => { | ||
| const checks = {}; | ||
| let healthy = true; | ||
|
|
||
| try { | ||
| checks.mongo = { status: 'ok', latencyMs: Math.round((await database.ping()) * 100) / 100 }; | ||
| } catch (err) { | ||
| // /health/ready is unauthenticated (a load balancer/orchestrator probe) | ||
| // — the same discipline as errorHandler.js applies: never put a raw | ||
| // driver error message in a public response. Mongo connection errors | ||
| // can include host/auth details a client has no business seeing. | ||
| logger.error('Readiness check: Mongo unreachable', { error: err.message }); | ||
| checks.mongo = { status: 'error' }; | ||
| healthy = false; | ||
| } | ||
|
|
||
| if (process.env.REDIS_URL) { | ||
| const client = getRedisClient(); | ||
| if (client && isRedisReady()) { | ||
| try { | ||
| const start = process.hrtime.bigint(); | ||
| await client.ping(); | ||
| checks.redis = { status: 'ok', latencyMs: Math.round((Number(process.hrtime.bigint() - start) / 1e6) * 100) / 100 }; | ||
| } catch (err) { | ||
| // Redis being down doesn't fail readiness — every Redis-backed | ||
| // feature (rate limiting, chat broadcast) already has a working | ||
| // in-memory/local fallback. Reported for visibility, not gating — | ||
| // and, same reasoning as the Mongo branch above, logged server-side | ||
| // rather than echoed to this unauthenticated endpoint's response. | ||
| logger.warn('Readiness check: Redis unreachable', { error: err.message }); | ||
| checks.redis = { status: 'degraded' }; | ||
| } | ||
| } else { | ||
| checks.redis = { status: 'connecting' }; | ||
| } | ||
| } else { | ||
| checks.redis = { status: 'not_configured' }; | ||
| } | ||
|
|
||
| res.status(healthy ? 200 : 503).json({ status: healthy ? 'ready' : 'not_ready', checks }); | ||
| }); |
…, and containerize API with Docker
…h, and password change flows
…e environment permissions
…file upload middleware
| this.collection.find(filter).sort(this.defaultSort).skip(skip).limit(limit).toArray(), | ||
| this.collection | ||
| .find(filter) | ||
| .sort(sort || this.defaultSort) |
…zation and add Claude project settings
…nd environment permissions settings
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
No description provided.