feat: comprehensive test suites, API auth/security, rule-files CRUD, projects tab#3
Conversation
…projects tab, and nav reorder Testing: - 12 new smoke test suites covering compiler, tool-registry, tool-detection, generic-configs, rule-files, skills, onboarding, config, http, api-docs, app-version - 101 API endpoint tests across 40+ routes Security: - Local API auth key: opt-in Bearer token auth for /api/* endpoints - CE_API_KEY env var override for headless setups - Token stored encrypted at rest in .keys.enc (AES-256-GCM) - Backup restore path traversal fix: timestamp validated against known backups Server: - server/lib/crypto.js: getAuthToken, setAuthToken, removeAuthToken, generateApiToken - server/server.js: auth middleware (Bearer check on /api/*, exempting /api/auth/*) - server/router.js: /api/auth/*, /api/rule-files/*, /api/projects/* endpoints - server/lib/rule-files.js: multi-file rule storage CRUD in data/rules/ - server/lib/backup.js: H1 path traversal fix — iso8601 regex validation UI: - Projects tab with grid/list toggle, per-project context scoping, publish rules - Rules tab receives rule-files dropdown for multi-file management - Nav: rules/soul tab moved before skills tab - CSS: projects tab styles decoupled from handoffs (tab-projects.css)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57995b36c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /** @param {string} name @returns {RuleData | null} */ | ||
| function readRuleFile(name) { | ||
| const filePath = path.join(RULES_DIR, `${name}.json`); |
There was a problem hiding this comment.
readRuleFile builds a path from untrusted input without applying sanitizeName, and the GET route passes decodeURIComponent output directly into it. A request like /api/rule-files/..%2F..%2Fmemory resolves outside data/rules and can read arbitrary JSON files on disk, which is a data exposure issue. Apply the same normalization/containment checks used by write/delete before reading.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed:
eadRuleFile now sanitizes the name via sanitizeName() and rejects paths containing .. — matches the hardening already in writeRuleFile and deleteRuleFile.
| const ruleList = document.getElementById('publish-rule-list'); | ||
| const targetList = document.getElementById('publish-target-list'); | ||
| if (ruleList) { | ||
| const result = await DS.getRuleFiles(); |
There was a problem hiding this comment.
This call throws at runtime because DS.getRuleFiles (and later DS.publishProjectRules) are invoked here, but those methods are not defined in ui/store.js's DS object. As soon as a user clicks “Publish Rules”, the modal flow breaks with a TypeError, so the new publish feature is unusable from the UI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: DS.getRuleFiles() and DS.publishProjectRules() added to ui/store.js — the publish modal now has the required data-layer methods and won't throw TypeError.
| if (p === '/api/rule-files' && req.method === 'POST') { | ||
| const { name, data } = await body(req); | ||
| if (!name) return json(res, { ok: false, error: 'Rule name is required' }, 400); | ||
| const v = validateRules(data); |
There was a problem hiding this comment.
The new rule-files POST route reuses validateRules(data), but that validator only accepts top-level string fields (coding/general/soul). The rule-files feature and combiner use structured objects (e.g. coding.hard/coding.soft), so valid create/update requests are rejected with 400 and cannot be persisted through this endpoint. Use a rule-file-specific validator (or broaden validateRules) for these routes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: validateRules now accepts both flat strings and nested hard/soft objects.
| if (token && url.pathname.startsWith('/api/') && !url.pathname.startsWith('/api/auth/')) { | ||
| const authHeader = String(req.headers.authorization || ''); | ||
| const provided = authHeader.startsWith('Bearer ') ? authHeader.slice(7) : ''; | ||
| if (provided !== token) { | ||
| res.writeHead(401, { 'Content-Type': 'application/json' }); | ||
| return res.end(JSON.stringify({ ok: false, error: 'Unauthorized: invalid or missing API token' })); |
There was a problem hiding this comment.
This middleware now rejects every /api/* request (except /api/auth/*) unless a Bearer token is present, but the web client request layer still sends only Content-Type headers and has no auth-token path. If CE_API_KEY is set (or generated once), normal UI flows immediately degrade to 401s across memory/rules/projects endpoints, effectively locking the UI out of its own API.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: apiFetch in ui/store.js now reads ce_auth_token from localStorage and sends Authorization: Bearer header on every request. 401 responses clear the token and show an error toast.
| const known = listBackups(); | ||
| if (!known.some((b) => b.timestamp === timestamp)) { |
There was a problem hiding this comment.
The new /api/restore guard now rejects any timestamp not returned by listBackups(), but that helper only exposes the newest 20 backups. As a result, valid older backup directories can no longer be restored through the API even though restoreBackup can handle them, which is a regression for long-running instances with more than 20 snapshots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: backup restore now falls back to direct directory lookup in data/backups/ with iso8601 regex guard when timestamp is not in the 20-entry listBackups() window.
| coding: { hard: 'test hard rule', soft: 'test soft rule' }, | ||
| general: { hard: '', soft: '' }, | ||
| soul: { soft: 'test soul' }, |
There was a problem hiding this comment.
This new smoke test posts object-shaped rules to /api/rules, but the server validator for that endpoint requires string fields (coding, general, soul). The test therefore fails on a healthy server and introduces persistent false negatives, which reduces the signal of the new API smoke suite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: with updated validateRules accepting nested objects, POST /api/rules correctly accepts the test's object payload. Test passes.
|
P1 (blocking):
P2 (should fix):
|
…en, validation P1: - readRuleFile: sanitize name (prevent ..%2F..%2F path traversal) - ui/store.js: add DS.getRuleFiles() and DS.publishProjectRules() - apiFetch: send Authorization Bearer header from localStorage - validateRules: accept both flat strings and nested priority objects P2: - Backup restore: fallback to direct directory lookup beyond 20-entry listBackups() window
|
Addressed all review items: P1 fixes:
P2 fixes:
|
Review: PR #3P1 (broken at runtime)1. readRuleFile path traversal
2. DS.getRuleFiles and DS.publishProjectRules are undefined
3. Auth middleware blocks UI when CE_API_KEY is set
4. POST /api/rule-files uses wrong validator
P2 (should fix)5. api-endpoints smoke test posts wrong schema to /api/rules
6. Backup restore guard only checks latest 20 backups
Test coverage by file
|
|
These issues were reported against the original commit \c57995b36c. All 6 are fixed in the current branch HEAD (\91ecea4\ → merge \51c2ec7): Verified on \origin/feat/tests-auth-security:
|
Updated review (branch: feat/tests-auth-security, HEAD: 51c2ec7)Previously identified - verified against fix commit (91ecea4)
Remaining gap: auth token has no UI (P1)The backend is fully wired:
The missing piece: nothing writes the token to localStorage.
Result: if auth is enabled, the UI 401s with no recovery path. Suggested approachA minimal config section in ui/config.js and ui/index.html that:
Everything else checks out. Merge-ready once the auth Settings UI is wired. |
- DS.getAuthStatus/generateAuthToken/removeAuthToken in ui/store.js - HTML section in ui/index.html with status, warning, token display, buttons - loadAuthStatus/updateAuthUI/generateAuthToken/removeAuthToken in ui/config.js - Writes token to localStorage so apiFetch Authorization header works end-to-end
|
Added minimal auth token Settings UI (commit d241856):
Flow: Settings loads → checks auth status → if not configured shows warning + Generate Token button → on click generates and stores token → apiFetch now sends Authorization: Bearer and 401s are handled. pm run check passes clean. |
2 participants
Summary
All changes from the original branch except the rules priority model refactor (now in #2).
Changes
Testing
compiler, tool-registry, tool-detection, generic-configs, rule-files,
skills, onboarding, config, http, api-docs, app-version
Security
/api/*endpoints(encrypted at rest in
.keys.encvia AES-256-GCM). When configured, UIwarns about unconfigured state.
CE_API_KEYenv var override for headless/scripted setupsknown backups + iso8601 regex
Server
server/lib/crypto.js: auth token lifecycle (get/set/remove/generateApiToken)server/server.js: auth middleware exempting/api/auth/*and static filesserver/lib/rule-files.js: multi-file rule storage CRUD indata/rules/server/router.js:/api/auth/*,/api/rule-files/*,/api/projects/*endpointsUI
tab-projects.css(not.handoffs-*selectors)Data Flow
data/rules/directory for per-name rule files (not committed, gitignored)