diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..5ace4600 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,6 @@ +version: 2 +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" diff --git a/.github/workflows/compat-smoke.yml b/.github/workflows/compat-smoke.yml index 73a8ae38..be1c96eb 100644 --- a/.github/workflows/compat-smoke.yml +++ b/.github/workflows/compat-smoke.yml @@ -2,17 +2,23 @@ on: push: + branches: [ main, master ] pull_request: + branches: [ main, master ] + +permissions: {} jobs: smoke: runs-on: ubuntu-latest + permissions: + contents: read strategy: matrix: python-version: ["3.10", "3.11", "3.12", "3.14"] steps: - - uses: actions/checkout@v4 - - uses: actions/setup-python@v5 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 with: python-version: ${{ matrix.python-version }} - name: Install deps @@ -28,12 +34,14 @@ jobs: smoke-windows: runs-on: windows-latest + permissions: + contents: read strategy: matrix: python-version: ["3.10", "3.12"] steps: - - uses: actions/checkout@v4 - - uses: actions/setup-python@v5 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 with: python-version: ${{ matrix.python-version }} - name: Install deps @@ -46,3 +54,14 @@ jobs: - name: Compat tests run: | python -m pytest -q tests/compat/test_import_smoke.py + + js-audit: + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - name: Install JS dependencies + run: npm ci --ignore-scripts + - name: Audit JS dependencies + run: npm audit --audit-level=moderate diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index f2630b40..ec700841 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -9,16 +9,18 @@ on: paths-ignore: - '**.md' - '.github/workflows/**' + - 'tests/**' -permissions: - contents: write +permissions: {} jobs: build-nightly: runs-on: ubuntu-latest + permissions: + contents: write steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Create Archive # Nous reproduisons ici la logique d'exclusion de votre script make_release_zip.ps1 @@ -32,10 +34,13 @@ jobs: -x "*/.pytest_cache/*" \ -x "tests/*" \ -x "scripts/make_release_zip.ps1" \ + -x "coverage_*.json" \ + -x "audit_findings.md" \ + -x ".vscode/*" \ -x "*.zip" - name: Update Nightly Release - uses: softprops/action-gh-release@v2 + uses: softprops/action-gh-release@f43927f8b9f890f7c14835e6ca0844af754a128d with: token: ${{ secrets.GITHUB_TOKEN }} tag_name: nightly diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index ab03834b..452fe806 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -11,21 +11,43 @@ on: paths: - "pyproject.toml" -permissions: - issues: write +permissions: {} jobs: + quality: + name: Quality Gate + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Check out code + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - name: Set up Python + uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + with: + python-version: "3.12" + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements.txt + pip install pytest pytest-asyncio + - name: Run tests + run: python -m pytest -q + publish-node: name: Publish Custom Node to registry runs-on: ubuntu-latest + needs: quality + permissions: + contents: read if: ${{ github.repository_owner == 'MajoorWaldi' }} steps: - name: Check out code - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: submodules: true - name: Publish Custom Node - uses: Comfy-Org/publish-node-action@v1 + uses: Comfy-Org/publish-node-action@d2366e798764f607447f2ef9d5ebcdd4d360f193 with: ## Add your own personal access token to your Github Repository secrets and reference it here. personal_access_token: ${{ secrets.REGISTRY_ACCESS_TOKEN }} diff --git a/.github/workflows/python-tests.yml b/.github/workflows/python-tests.yml index 32cd3c56..4cd6e1e1 100644 --- a/.github/workflows/python-tests.yml +++ b/.github/workflows/python-tests.yml @@ -6,14 +6,18 @@ on: pull_request: branches: [ main, master ] +permissions: {} + jobs: quality: runs-on: ubuntu-22.04 + permissions: + contents: read steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 with: python-version: "3.12" @@ -21,7 +25,7 @@ jobs: run: | python -m pip install --upgrade pip pip install -r requirements.txt - pip install xenon radon flake8 flake8-cognitive-complexity + pip install xenon radon flake8 flake8-cognitive-complexity pip-audit bandit - name: Check Python files encoding (no UTF-8 BOM) run: python scripts/check_no_bom.py mjr_am_backend mjr_am_shared tests __init__.py scripts @@ -32,17 +36,31 @@ jobs: - name: Complexity gate (new files, CC <= 10) run: python scripts/check_cc_changed.py + - name: Audit Python dependencies + run: pip-audit --strict + + - name: SAST scan (bandit) + run: bandit -r mjr_am_backend -ll -ii -x tests + + - name: Install JS dependencies for audit + run: npm ci --ignore-scripts + + - name: Audit JS dependencies + run: npm audit --audit-level=high + test: runs-on: ubuntu-22.04 + permissions: + contents: read strategy: fail-fast: false matrix: python-version: ["3.10", "3.12", "3.13", "3.14"] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v5 + uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 with: python-version: ${{ matrix.python-version }} @@ -69,11 +87,13 @@ jobs: typecheck: runs-on: ubuntu-22.04 + permissions: + contents: read steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 with: python-version: "3.12" diff --git a/.github/workflows/release-on-tag.yml b/.github/workflows/release-on-tag.yml index bade9cc3..2d80a910 100644 --- a/.github/workflows/release-on-tag.yml +++ b/.github/workflows/release-on-tag.yml @@ -5,16 +5,17 @@ on: tags: - "v*.*.*" # Se déclenche sur les tags comme v1.0.0, v0.2.1, etc. -permissions: - contents: write +permissions: {} jobs: create-release: name: Create GitHub Release runs-on: ubuntu-latest + permissions: + contents: write steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: submodules: true @@ -33,7 +34,7 @@ jobs: -x "*.zip" - name: Create Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@f43927f8b9f890f7c14835e6ca0844af754a128d with: files: ComfyUI-Majoor-AssetsManager_release.zip generate_release_notes: true @@ -47,14 +48,16 @@ jobs: needs: create-release name: Publish to Comfy Registry runs-on: ubuntu-latest + permissions: + contents: read # Condition extraite de votre fichier publish.yml original if: ${{ github.repository_owner == 'MajoorWaldi' }} steps: - name: Check out code - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: submodules: true - name: Publish Custom Node - uses: Comfy-Org/publish-node-action@v1 + uses: Comfy-Org/publish-node-action@d2366e798764f607447f2ef9d5ebcdd4d360f193 with: - personal_access_token: ${{ secrets.REGISTRY_ACCESS_TOKEN }} \ No newline at end of file + personal_access_token: ${{ secrets.REGISTRY_ACCESS_TOKEN }} diff --git a/docs/DOCUMENTATION_INDEX.md b/docs/DOCUMENTATION_INDEX.md index 84dda2fc..aceea9e1 100644 --- a/docs/DOCUMENTATION_INDEX.md +++ b/docs/DOCUMENTATION_INDEX.md @@ -22,6 +22,9 @@ Welcome to the comprehensive documentation for the Majoor Assets Manager for Com - [TESTING.md](TESTING.md) - Running tests and reading reports - [DB_MAINTENANCE.md](DB_MAINTENANCE.md) - Database maintenance, corruption recovery, and Delete DB +### Security Audits +- [SECURITY_AUDIT_2026-02-27.md](SECURITY_AUDIT_2026-02-27.md) - Full security & code quality audit (4 CRITICAL · 11 HIGH · 14 MEDIUM) + ### Complete Guides - [COMPLETE_USER_GUIDE.md](COMPLETE_USER_GUIDE.md) - Step-by-step user guide with workflows diff --git a/docs/SECURITY_AUDIT_2026-02-27.md b/docs/SECURITY_AUDIT_2026-02-27.md new file mode 100644 index 00000000..d77e399c --- /dev/null +++ b/docs/SECURITY_AUDIT_2026-02-27.md @@ -0,0 +1,933 @@ +# Security & Code Quality Audit — 2026-02-27 + +Full deep audit of the entire codebase (Python backend, JavaScript frontend, CI/CD, tests). +Conducted with static analysis of every source file. + +--- + +## Summary + +| Severity | Count | +|---|---| +| CRITICAL | 4 | +| HIGH | 11 | +| MEDIUM | 14 | +| LOW | 10 | +| INFO / Closed | 6 | +| **Total** | **45** | + +### Progress vs. previous audit (2026-02-26) + +| Previous finding | Status | +|---|---| +| Path traversal on restore backup — `db_maintenance.py` | **FIXED** | +| `asyncio_mode` missing in `pytest.ini` | **FIXED** | +| Dev/test deps in prod `requirements.txt` | **FIXED** | +| SSRF via unvalidated `owner`/`repo` — `releases.py` | Mitigated; residual gap (H-01) | +| SQL injection PRAGMA — `db_recovery.py` | Partially guarded; f-string persists (C-01) | +| Token plaintext in rotate response | Partially mitigated (C-02) | +| `window.open(undefined)` — `GenerationSection.js` | Empty check added; scheme not validated (C-03) | +| Unbounded `while(true)` retry loops | Bounded to 10 attempts; no backoff (H-03) | +| Workflow JSON loaded without validation | Shape check added; content not validated (H-04) | +| GitHub Actions not SHA-pinned | Third-party actions pinned; `actions/*` still tag-pinned (H-08) | +| `filtersController.js` listener leak | `lifecycleSignal` added; disposal gap remains (H-10) | + +--- + +## CRITICAL + +--- + +### C-01 — SQL injection via f-string in `PRAGMA table_info` + +**Files:** `mjr_am_backend/adapters/db/schema.py:204,288` · `mjr_am_backend/adapters/db/db_recovery.py:135` + +**Description:** +`PRAGMA table_info` is executed using f-strings with single-quote wrapping around the table +name — `f"PRAGMA table_info('{table_name}')"`. SQLite does not support bind parameters in PRAGMA +statements, but single-quote wrapping is insufficient protection: a value like `assets');DROP TABLE +assets;--` breaks out of the string. The guard `_is_safe_identifier` is the sole protection, applied +only at call-sites, not enforced inside the functions that execute the PRAGMA. + +**Impact:** If any future call path reaches these sites without going through the guard (a refactor, +a new caller, a copy-paste), arbitrary SQL can be injected into the database. + +**Suggested fix:** + +1. Switch to double-quote identifier escaping, which is the SQL standard for identifiers: + ```python + # Before + f"PRAGMA table_info('{table_name}')" + + # After — double-quote identifier, escape embedded double-quotes + safe = table_name.replace('"', '""') + f'PRAGMA table_info("{safe}")' + ``` + +2. Add the validation gate at the start of each function that executes the PRAGMA, not only + at call-sites: + ```python + def _get_table_columns(cursor, table_name: str) -> list[str]: + if not _is_safe_identifier(table_name): + raise ValueError(f"Unsafe table name: {table_name!r}") + safe = table_name.replace('"', '""') + cursor.execute(f'PRAGMA table_info("{safe}")') + ... + ``` + +3. Since all tables are statically known, consider a hard allowlist: + ```python + _KNOWN_TABLES = frozenset({"assets", "collections", "tags", ...}) + + def _get_table_columns(cursor, table_name: str) -> list[str]: + if table_name not in _KNOWN_TABLES: + raise ValueError(f"Unknown table: {table_name!r}") + ... + ``` + +--- + +### C-02 — API token returned plaintext in JSON response body + +**Files:** `mjr_am_backend/routes/handlers/health.py:583,622` · `js/api/client.js:123-136` + +**Description:** +The `rotate-token` and `bootstrap-token` endpoints return the raw token in the response body +(`{"token": "...", "api_token": "..."}`). The transport-security check allows plain HTTP when the +client is on loopback, so the token can be transmitted unencrypted. The JS client then persists it +to `localStorage` in plaintext, where any same-origin JavaScript (XSS, extensions) can read it. +Once exfiltrated, the token authorizes all write, delete, and rename operations. + +**Suggested fix:** + +1. Return the token only once (at creation/rotation). On subsequent reads, return only a masked + hint (e.g., `"••••••••" + last4`), never the full value: + ```python + # Only include raw token in the rotation/creation response, never in GET /status + if is_new_token: + return {"token": new_token, ...} + else: + return {"token_hint": f"...{stored_token[-4:]}", ...} + ``` + +2. Consider using an `httpOnly` session cookie for the token instead of returning it in JSON. + The browser will send it automatically and JavaScript cannot access it: + ```python + response.set_cookie("mjr_write_token", token, httponly=True, samesite="Strict", + secure=not is_loopback) + ``` + +3. If `localStorage` must be used, document the risk explicitly and enforce a Content Security + Policy (CSP) header to reduce XSS attack surface: + ```python + response.headers["Content-Security-Policy"] = ( + "default-src 'self'; script-src 'self'; object-src 'none';" + ) + ``` + +4. Log a warning when the loopback HTTP exception is used: + ```python + if is_loopback and not is_https: + logger.warning("Token transmitted over plain HTTP on loopback — acceptable only for local dev") + ``` + +--- + +### C-03 — `window.open(src)` with server-provided URL — scheme not validated + +**File:** `js/components/sidebar/sections/GenerationSection.js:537-539` + +**Description:** +`srcCandidates[0]` is passed directly to `window.open(src, "_blank")`. The empty-string check +added in a previous fix prevents the `undefined` crash but does not validate the URL scheme. +A `javascript:` URI or `data:text/html,...` URI returned by the backend (e.g., via a compromised +database or SQL injection) would execute arbitrary JavaScript in a new tab. + +**Suggested fix:** + +Add a scheme allowlist before calling `window.open`. Relative paths (served by the same origin) +are safe; only `http:` and `https:` absolute URLs should be allowed: + +```javascript +// Before +if (src) { + window.open(src, "_blank"); +} + +// After +function _isSafeUrl(url) { + if (!url) return false; + // Allow relative paths (same origin) + if (url.startsWith('/')) return true; + // Allow only http/https absolute URLs + try { + const parsed = new URL(url); + return parsed.protocol === 'http:' || parsed.protocol === 'https:'; + } catch { + return false; + } +} + +if (_isSafeUrl(src)) { + window.open(src, "_blank", "noopener,noreferrer"); +} +``` + +Note: always add `noopener,noreferrer` to `window.open` for `_blank` targets to prevent the opened +page from accessing the opener via `window.opener`. + +--- + +### C-04 — Authentication bypass race on `bootstrap-token` + +**File:** `mjr_am_backend/routes/handlers/health.py:586-594` + +**Description:** +The authorization logic is: +```python +auth = _require_write_access(request) +if not auth.ok: + user_auth = _require_authenticated_user(request) + if not (user_auth.ok and _is_loopback_request(request)): + return _json_response(auth) +``` +When no token is configured yet, `_require_write_access` fails. The fallback allows any Comfy +user on loopback. When Comfy auth is disabled, `_require_authenticated_user` returns `Ok("")`. +This means any process on the local machine can call `POST /bootstrap-token` and mint a write +token with no authentication at all. + +**Suggested fix:** + +1. Require explicit opt-in for bootstrap via an environment variable that is removed after + first use: + ```python + # In health.py bootstrap handler + if os.environ.get("MAJOOR_ALLOW_BOOTSTRAP") != "1": + return _json_response(Result.Err("BOOTSTRAP_DISABLED", + "Set MAJOOR_ALLOW_BOOTSTRAP=1 to enable initial token creation")) + # After issuing the token, instruct the user to unset the variable + ``` + +2. Alternatively, require that the bootstrap endpoint can only be called once (one-time nonce): + ```python + _BOOTSTRAP_DONE = False # module-level flag + + async def bootstrap_token(request): + global _BOOTSTRAP_DONE + if _BOOTSTRAP_DONE: + return _json_response(Result.Err("ALREADY_BOOTSTRAPPED", "Token already exists")) + ... + _BOOTSTRAP_DONE = True + ``` + +3. Add explicit documentation in `SECURITY_ENV_VARS.md` explaining the loopback + auth-disabled + combination and its implications. + +--- + +## HIGH + +--- + +### H-01 — Residual SSRF in `releases.py` — `.` allowed in owner/repo regex + +**File:** `mjr_am_backend/routes/handlers/releases.py:80-86` + +**Description:** +`_SAFE_GITHUB_SEGMENT_RE = re.compile(r"^[a-zA-Z0-9_.-]{1,100}$")` allows `.`, which means `..` +is a valid segment. While GitHub's routing would likely ignore this, the pattern creates an +inconsistency and could allow path traversal in future URL constructions that reuse this validator. + +**Suggested fix:** + +```python +# Before +_SAFE_GITHUB_SEGMENT_RE = re.compile(r"^[a-zA-Z0-9_.-]{1,100}$") + +# After — GitHub owner/repo names cannot start/end with dots or contain consecutive dots +_SAFE_GITHUB_SEGMENT_RE = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9_-]{0,99}$") +# Also add an explicit check +if ".." in owner or ".." in repo: + return _json_response(Result.Err("INVALID_INPUT", "Invalid owner or repo name")) +``` + +--- + +### H-02 — Cleanup daemon thread has no stop signal and swallows all exceptions + +**File:** `mjr_am_backend/routes/handlers/batch_zip.py:104-111` + +**Description:** +`_run_cleanup_loop()` runs `while True: time.sleep(interval); _cleanup_batch_zips()` in a daemon +thread. There is no way to stop it cleanly on shutdown. Any exception in `_cleanup_batch_zips` is +silently swallowed with `except Exception: continue`, meaning repeated failures (disk full, +permission error) produce no log output and no backoff. + +**Suggested fix:** + +```python +_CLEANUP_STOP = threading.Event() + +def _run_cleanup_loop(interval: float = 300.0) -> None: + consecutive_errors = 0 + while not _CLEANUP_STOP.wait(timeout=min(interval * (2 ** consecutive_errors), 3600)): + try: + _cleanup_batch_zips() + consecutive_errors = 0 + except Exception as exc: + consecutive_errors += 1 + logger.warning("Batch ZIP cleanup error (attempt %d): %s", consecutive_errors, exc) + +def start_cleanup_thread() -> None: + t = threading.Thread(target=_run_cleanup_loop, daemon=True, name="batch-zip-cleanup") + t.start() + return t + +def stop_cleanup_thread() -> None: + _CLEANUP_STOP.set() +``` + +--- + +### H-03 — Retry loop fires up to 10 API calls with no delay + +**Files:** `js/components/RatingEditor.js:53` · `js/components/TagsEditor.js:247` + +**Description:** +`while (attempts < 10)` with `attempts += 1` and no `await delay(...)` between iterations. +When the abort/retry cycle triggers, up to 10 API calls are issued back-to-back in the same +microtask queue flush, amplifying rate-limit consumption and backend load. + +**Suggested fix:** + +Add exponential backoff between retry attempts: + +```javascript +// Add a helper (e.g., in js/utils/debounce.js or inline) +const delay = (ms) => new Promise(resolve => setTimeout(resolve, ms)); + +// Inside flushSaves() / the retry loop +let attempts = 0; +while (attempts < 10) { + if (attempts > 0) { + await delay(Math.min(100 * 2 ** (attempts - 1), 2000)); // 100ms, 200ms, 400ms … max 2s + } + attempts += 1; + // ... existing retry logic +} +``` + +--- + +### H-04 — Workflow JSON loaded into ComfyUI without content validation + +**File:** `js/features/dnd/DragDrop.js:40-186` + +**Description:** +`isValidWorkflowShape(workflow)` checks only that `workflow.nodes` and `workflow.links` are arrays +and that the total serialized size is within bounds. It does not validate the content of individual +nodes (types, widget values, properties). A compromised database or SQL injection could return a +crafted workflow that triggers unintended ComfyUI node execution when dropped. + +**Suggested fix:** + +Add a deeper structural validation pass before calling `app.loadGraphData`: + +```javascript +const ALLOWED_NODE_TYPES_PREFIX = [ + "KSampler", "CLIPTextEncode", "VAEDecode", "LoraLoader", + "CheckpointLoaderSimple", "SaveImage", "PreviewImage", + // ... extend with known-good node type prefixes +]; + +function _isSafeWorkflowNode(node) { + if (!node || typeof node !== 'object') return false; + if (typeof node.id !== 'number') return false; + // Reject nodes with no recognized type prefix (optional strictness) + // if (node.type && !ALLOWED_NODE_TYPES_PREFIX.some(p => node.type.startsWith(p))) return false; + // Reject nodes with suspicious string values in widgets_values + if (Array.isArray(node.widgets_values)) { + for (const v of node.widgets_values) { + if (typeof v === 'string' && v.length > 8192) return false; // reject huge strings + } + } + return true; +} + +function isValidWorkflowShape(workflow) { + // ... existing checks ... + if (!workflow.nodes.every(_isSafeWorkflowNode)) return false; + return true; +} +``` + +Additionally, call `window.open` with the workflow URL using `noopener,noreferrer` to isolate the +context if workflow loading is ever done via URL navigation. + +--- + +### H-05 — `innerHTML` pattern established in shared utilities + +**Files:** `js/components/Viewer_impl.js:539` · `js/components/sidebar/utils/dom.js:91,101,103` + +**Description:** +Several helpers assign to `innerHTML` using hardcoded string literals, which is safe today. +However, this establishes a pattern that will likely be followed by future contributors with +user-controlled data, and there is no ESLint rule preventing it. + +**Suggested fix:** + +Add an ESLint rule (or comment directive) to the project: + +```json +// In package.json or .eslintrc +{ + "rules": { + "no-unsanitized/property": "error" // requires eslint-plugin-no-unsanitized + } +} +``` + +Or add a project-wide comment convention above every legitimate `innerHTML` use: + +```javascript +// safe: static literal, no user data +container.innerHTML = '...'; +``` + +And add a `grep`-based CI check that flags any `innerHTML` assignment containing template +literals with variable interpolation. + +--- + +### H-06 — API token stored in `localStorage` in plaintext + +**File:** `js/api/client.js:123-136` + +**Description:** +The write-access token is persisted to `localStorage` under the settings key. `localStorage` is +accessible to all JavaScript in the same origin, including any XSS payload or malicious browser +extension. An attacker who achieves XSS once can extract the token and make authenticated write +requests indefinitely. + +**Suggested fix:** + +1. If the token must live in the browser, prefer `sessionStorage` over `localStorage` (cleared + when the tab closes, reducing the window of exposure): + ```javascript + // Use sessionStorage instead of localStorage for the token + sessionStorage.setItem(TOKEN_KEY, token); + ``` + +2. Better: move the token to an `httpOnly` cookie managed by the server (see C-02 fix). + +3. Enforce a Content Security Policy on the ComfyUI extension response to limit XSS impact. + +4. Add token rotation on detection of suspicious activity (multiple failed requests from + different IPs with the same token). + +--- + +### H-08 — `actions/checkout` and `actions/setup-python` pinned by tag, not SHA + +**Files:** `.github/workflows/python-tests.yml` · `.github/workflows/compat-smoke.yml` · +`.github/workflows/nightly.yml` · `.github/workflows/release-on-tag.yml` + +**Description:** +`actions/checkout@v4` and `actions/setup-python@v5` use mutable version tags. A tag can be +moved at any time by GitHub or the action maintainer, silently executing different code in CI. + +**Suggested fix:** + +Pin both actions to their full commit SHA. Example (verify the actual SHA from the GitHub +Actions marketplace before using): + +```yaml +# actions/checkout v4 (as of early 2026) +- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + +# actions/setup-python v5 (as of early 2026) +- uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 +``` + +Add a Dependabot configuration to keep these automatically up to date: + +```yaml +# .github/dependabot.yml +version: 2 +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" +``` + +--- + +### H-09 — Workflows missing explicit deny-all permissions baseline + +**File:** `.github/workflows/nightly.yml:15` + +**Description:** +`permissions: contents: write` is declared for the release step but there is no top-level +`permissions: {}` deny-all baseline. Any step that does not need write access still has it +implicitly. A supply-chain compromise of any action used in the same workflow could push code +or create releases. + +**Suggested fix:** + +Add a top-level `permissions: {}` deny-all to every workflow, then grant only what each job +actually needs: + +```yaml +# Top-level deny-all +permissions: {} + +jobs: + release: + permissions: + contents: write # only this job needs write + steps: + ... + + tests: + permissions: + contents: read # tests only need to read the repo + steps: + ... +``` + +--- + +### H-10 — `filtersController.js` — listener leak when `lifecycleSignal` is provided + +**File:** `js/features/panel/controllers/filtersController.js:29-50` + +**Description:** +When `lifecycleSignal` is provided to `addManagedListener`, the listener is registered on the +signal for cleanup but the `disposers` array is not populated. The `dispose()` function returned +by `bindFilters` only runs functions in `disposers`, so it is a no-op when signals are in use. +If the panel is remounted while the signal is still active, duplicate listeners accumulate on +live DOM elements, causing multiple grid reloads per user interaction. + +**Suggested fix:** + +Two complementary fixes: + +1. Ensure `dispose()` is called in the panel teardown path, even when signals handle most + cleanup (defensive belt-and-suspenders): + ```javascript + // In AssetsManagerPanel.js or wherever the panel lifecycle is managed + const disposeFilters = bindFilters(els, opts, signal); + signal.addEventListener('abort', disposeFilters, { once: true }); + ``` + +2. Make `addManagedListener` also push to `disposers` when a signal is provided, so that + explicit `dispose()` calls always work: + ```javascript + function addManagedListener(target, event, handler, opts) { + const signal = opts?.signal; + target.addEventListener(event, handler, opts); + const remove = () => target.removeEventListener(event, handler, opts); + disposers.push(remove); // always push — belt and suspenders + if (signal) { + signal.addEventListener('abort', remove, { once: true }); + } + } + ``` + +--- + +### H-11 — `PRAGMA cache_size` / `PRAGMA busy_timeout` use f-string interpolation pattern + +**File:** `mjr_am_backend/adapters/db/sqlite_facade.py:765,767` + +**Description:** +```python +f"PRAGMA cache_size={SQLITE_CACHE_SIZE_KIB}" +f"PRAGMA busy_timeout = {SQLITE_BUSY_TIMEOUT_MS}" +``` +These are safe today because both constants are derived from `int()` conversions of environment +variables at module load. However, the pattern is dangerous: any future developer who copies +this PRAGMA template and substitutes a non-integer value introduces an injection. + +**Suggested fix:** + +Add inline comments and assert the type before interpolation: + +```python +assert isinstance(SQLITE_CACHE_SIZE_KIB, int), "must be int — never interpolate user input into PRAGMA" +cursor.execute(f"PRAGMA cache_size={SQLITE_CACHE_SIZE_KIB}") # int constant only + +assert isinstance(SQLITE_BUSY_TIMEOUT_MS, int), "must be int — never interpolate user input into PRAGMA" +cursor.execute(f"PRAGMA busy_timeout={SQLITE_BUSY_TIMEOUT_MS}") # int constant only +``` + +--- + +## MEDIUM + +--- + +### M-02 — `os.startfile` on Windows invokes shell handlers + +**File:** `mjr_am_backend/routes/handlers/assets_impl.py:665` + +```python +os.startfile(str(resolved.parent)) +``` + +`os.startfile` uses the Windows shell association for the given path. A directory with a crafted +`desktop.ini` file could trigger an unexpected shell handler. Prefer an explicit command: + +```python +# Instead of os.startfile(str(resolved.parent)) +import subprocess +subprocess.Popen(["explorer.exe", str(resolved.parent)], shell=False) +``` + +--- + +### M-03 — Exception swallowing in batch ZIP cleanup thread + +**File:** `mjr_am_backend/routes/handlers/batch_zip.py:107-111` + +```python +# Before +except Exception: + continue + +# After +except Exception as exc: + logger.warning("Batch ZIP cleanup failed: %s", exc, exc_info=True) +``` + +--- + +### M-04 — TOCTOU race between `final.exists()` and `tmp.replace(final)` during upload + +**File:** `mjr_am_backend/routes/handlers/scan_helpers.py:232-239` + +Between `while final.exists()` and `tmp_obj.replace(final)`, another concurrent upload can +claim the same filename. Fix with an atomic claim: + +```python +# Atomically claim the destination filename before moving the temp file +try: + final.open('x').close() # O_EXCL: fails if another process just created it +except FileExistsError: + n += 1 + continue +# Now exclusively own `final`; move temp file +tmp_obj.replace(final) +``` + +--- + +### M-05 — Custom roots registered with `resolve(strict=False)` + +**File:** `mjr_am_backend/custom_roots.py:85` + +A custom root that doesn't exist yet passes validation silently. Use `strict=True` to require +the directory to exist at registration time: + +```python +# Before +resolved = p.resolve(strict=False) + +# After +try: + resolved = p.resolve(strict=True) +except (FileNotFoundError, OSError) as exc: + return Result.Err("INVALID_PATH", f"Custom root path does not exist: {exc}") +``` + +--- + +### M-06 — Unbounded ZIP filename collision loop + +**File:** `mjr_am_backend/routes/handlers/batch_zip.py:409-415` + +When many files share the same stem and all truncated names are identical (stem fills the +255-char limit), the loop never terminates. Add a safety exit: + +```python +import uuid + +n = 1 +while True: + attempt = f"{stem} ({n}){suffix}" + attempt = attempt[:_ZIP_NAME_MAX_LEN] + if attempt not in used_names: + candidate = attempt + break + n += 1 + if n > len(items) + 200: # impossible collision — use UUID fallback + candidate = f"{uuid.uuid4().hex[:12]}{suffix}" + break +``` + +--- + +### M-08 — Rate limiter fails open on exception + +**File:** `mjr_am_backend/routes/core/security.py:803-804` + +```python +# Before +except Exception: + return True, None + +# After +except Exception as exc: + logger.error("Rate limit check raised unexpectedly, failing open: %s", exc, exc_info=True) + return True, None +``` + +Consider whether the bootstrap-token endpoint should instead fail **closed** (deny the +request) when rate-limit state cannot be read. + +--- + +### M-09 — `_get_client_identifier` uses `XFF[0]` — spoofable behind a proxy + +**File:** `mjr_am_backend/routes/core/security.py:691-710` + +`X-Forwarded-For` position 0 is client-controlled. The correct function +`_client_ip_from_forwarded_chain` (used in `_check_write_access`) trims trusted proxies from +the right to find the last untrusted hop. + +```python +# In _get_client_identifier, replace: +forwarded_for.split(",")[0].strip() + +# With: +_resolve_client_ip(request) # chain-trimming version +``` + +--- + +### M-10 — No SAST or dependency vulnerability scanning in CI + +**Files:** `.github/workflows/python-tests.yml` · `.github/workflows/compat-smoke.yml` + +Add the following steps to the quality job in `python-tests.yml`: + +```yaml +- name: Audit Python dependencies + run: pip audit --strict + +- name: SAST scan (bandit) + run: bandit -r mjr_am_backend -ll -ii -x mjr_am_backend/tests + +- name: Audit JS dependencies + run: npm audit --audit-level=high +``` + +Add `pip-audit` and `bandit` to `[project.optional-dependencies] dev` in `pyproject.toml`. + +--- + +### M-13 — Rate-limit cleanup O(N×M) under global lock + +**File:** `mjr_am_backend/routes/core/security.py:751-760` + +`_cleanup_rate_limit_state_locked` iterates all clients × all endpoints inside `_rate_limit_lock`. +With 1 000 clients × 32 endpoints this is 32 000 list comprehensions per 100 requests. + +**Suggested fix:** Move cleanup to a background thread that runs every 60 seconds, holding the +lock only for the final bulk-delete of expired keys: + +```python +def _cleanup_rate_limit_background() -> None: + """Run in a daemon thread; prune expired rate-limit entries periodically.""" + while True: + time.sleep(60) + now = time.monotonic() + with _rate_limit_lock: + to_delete = [ + (client, ep) + for client, eps in _rate_limit_state.items() + for ep, window in eps.items() + if window.expired(now) + ] + for client, ep in to_delete: + del _rate_limit_state[client][ep] + if not _rate_limit_state[client]: + del _rate_limit_state[client] +``` + +--- + +### M-14 — Content-Disposition header does not strip low ASCII control characters + +**File:** `mjr_am_backend/routes/handlers/batch_zip.py:184-185` + +```python +# Before — strips only `"`, `;`, CR, LF +safe_name = str(name).replace('"', '').replace(';', '').replace('\r', '').replace('\n', '')[:255] + +# After — keep only printable ASCII (0x20–0x7e) +import re +safe_name = re.sub(r'[^\x20-\x7e]', '_', str(name))[:255] +``` + +--- + +### MF-01 — Negative workflow fetch results cached for 60 seconds + +**File:** `js/features/dnd/DragDrop.js:146-149` + +A transient network error on first drag-over caches a negative result for a full minute. +Do not cache failures (or use a very short TTL): + +```javascript +// Before +_workflowCache.set(cacheKey, { workflow: null, ts: Date.now() }); + +// After — only cache successful results; let failures fall through for immediate retry +if (workflow !== null) { + _workflowCache.set(cacheKey, { workflow, ts: Date.now() }); +} +``` + +--- + +### MT-01 — No integration tests for real CSRF and auth paths + +**File:** `tests/features/test_health_routes.py:123,273,360` + +All write-endpoint tests monkeypatch `_csrf_error` and `_require_write_access` to pass +unconditionally. A regression in either guard would not be caught. + +**Suggested fix:** Add at least one integration test per write endpoint using +`aiohttp.test_utils.TestClient` with real middleware, verifying that: +- A request missing the anti-CSRF header returns `CSRF_ERROR` +- A request without a valid token returns `AUTH_REQUIRED` + +--- + +### MT-02 / MT-03 — No tests for `releases.py` or `batch_zip.py` + +Create `tests/features/test_releases_routes.py` covering: +- `owner`/`repo` values containing `..`, `@`, spaces → rejected with 400 +- Valid owner/repo → mock HTTP call returns expected shape + +Create `tests/features/test_batch_zip_routes.py` covering: +- Items exceeding `MAX_BATCH_ZIP_ITEMS` → rejected +- Path traversal in `filename`/`subfolder` → rejected +- Concurrent creation of the same batch → no overwrite + +--- + +## LOW + +--- + +### L-02 — Nightly release ZIP includes internal files + +**File:** `.github/workflows/nightly.yml:28` + +The nightly ZIP includes `coverage_backend.json`, `audit_findings.md`, and `.vscode/` settings. +Add exclusions: + +```yaml +# In the zip creation step +zip -r dist/... . \ + -x ".claude/*" \ + -x "coverage_*.json" \ + -x "audit_findings.md" \ + -x ".vscode/*" \ + -x "*.pyc" \ + -x "__pycache__/*" +``` + +--- + +### L-04 — Upload size limit checked after writing first chunk + +**File:** `mjr_am_backend/routes/handlers/scan_helpers.py:219-229` + +Check `Content-Length` before starting the write loop to reject oversized uploads eagerly: + +```python +content_length = request.content_length +if content_length is not None and content_length > _MAX_UPLOAD_SIZE: + return Result.Err("FILE_TOO_LARGE", f"Upload exceeds {_MAX_UPLOAD_SIZE} bytes") +# Then proceed with chunk loop as a second guard +``` + +--- + +### L-07 — Auth check order: `_require_write_access` after `_require_services` + +**File:** `mjr_am_backend/routes/handlers/assets_impl.py:554-567` + +Move the write-access check before the services check so that unauthenticated clients cannot +probe service availability: + +```python +# Recommended order for all write handlers +csrf = _csrf_error(request) +if csrf: return csrf +auth = _require_write_access(request) +if not auth.ok: return _json_response(auth) +svc = _require_services(request) +if not svc.ok: return _json_response(svc) +op = _require_operation_enabled(request, ...) +if not op.ok: return _json_response(op) +``` + +--- + +### L-08 — `npm audit` never run + +Add `npm audit --audit-level=moderate` to the JS test workflow and to local development +tooling (`run_tests.bat` or a new `scripts/audit.sh`). + +--- + +### LF-01 — Dead code: `agendaHandler` variable + +**File:** `js/features/panel/controllers/filtersController.js:28` + +```javascript +// Remove this unused declaration +let agendaHandler = null; +``` + +--- + +## INFO — Closed / Verified Good + +| ID | Finding | Status | +|---|---|---| +| I-01 | `asyncio_mode = auto` present in `pytest.ini` | Closed / Good | +| I-02 | `requirements.txt` is free of dev/test tools | Closed / Good | +| I-03 | `pyproject.toml` uses `[project.optional-dependencies] dev` correctly | Closed / Good | +| I-04 | `pytest.ini` uses `norecursedirs` to exclude temp dirs | Closed / Good | +| I-05 | All PRAGMA sites in `schema.py` guarded by `_is_safe_identifier` | Closed / Partial (see C-01) | +| I-06 | `_check_write_access` uses `hmac.compare_digest` for timing-attack resistance | Closed / Good | + +--- + +## Remediation Priority Order + +| Priority | ID | Effort | Impact | +|---|---|---|---| +| 1 | C-03 | Low (2 lines) | Prevents stored XSS via crafted metadata | +| 2 | C-01 | Low (add assert + double-quote) | Hardens SQL injection defence | +| 3 | H-01 | Low (1 line regex change) | Closes SSRF residual gap | +| 4 | M-14 | Low (1 line regex) | Fixes header injection | +| 5 | LF-01 | Trivial | Remove dead code | +| 6 | H-02 | Medium | Fixes silent cleanup failure + shutdown hang | +| 7 | M-06 | Low | Prevents theoretical infinite loop in ZIP builder | +| 8 | M-08 | Low (add log line) | Surfaces rate-limit failures | +| 9 | M-09 | Medium | Prevents rate-limit bypass via XFF spoofing | +| 10 | H-03 | Low | Reduces burst API call load | +| 11 | C-04 | Medium | Tightens bootstrap-token auth | +| 12 | C-02 / H-06 | High (architectural) | Removes token from response body / localStorage | +| 13 | H-04 | Medium | Deep workflow validation | +| 14 | MT-01/02/03 | Medium | Integration test coverage for security paths | +| 15 | M-10 | Low (CI config) | Adds dependency vuln scanning | +| 16 | H-08 / H-09 | Low (CI config) | SHA pins + permission scoping | diff --git a/docs/SECURITY_ENV_VARS.md b/docs/SECURITY_ENV_VARS.md index 2defc980..ba01c805 100644 --- a/docs/SECURITY_ENV_VARS.md +++ b/docs/SECURITY_ENV_VARS.md @@ -45,9 +45,10 @@ To reduce risk, Majoor blocks destructive/write operations from non-local client #### Overrides (use with care) - `MAJOOR_REQUIRE_AUTH=1` forces token auth even for loopback (requires `MAJOOR_API_TOKEN`). - `MAJOOR_ALLOW_REMOTE_WRITE=1` allows remote write operations without a token (**unsafe**). +- `MAJOOR_ALLOW_BOOTSTRAP=1` temporarily enables initial `/bootstrap-token` provisioning. Disable it after first successful bootstrap. -### Client-side token storage -Set the same secret inside ComfyUI's Settings modal at **Security → Majoor: API Token**. The Assets Manager saves it in browser storage and automatically includes both `X-MJR-Token` and `Authorization: Bearer …` on outbound requests, so remote clients always send the expected header without manual tooling. The backend still needs the matching `MAJOOR_API_TOKEN`/`MJR_API_TOKEN` environment variable at startup for write endpoints to accept the secret. +### Client-side token handling +Set the same secret inside ComfyUI's Settings modal at **Security -> Majoor: API Token**. Runtime tokens are kept in `sessionStorage` (tab/session scoped) and bootstrap/rotate responses set an `httpOnly` cookie (`mjr_write_token`) so JavaScript does not need to read plaintext secrets from API responses. ### Safe Mode (default enabled) Safe Mode adds an explicit opt-in layer for operations that modify files or user metadata. diff --git a/js/api/client.js b/js/api/client.js index 65447f73..01e45a91 100644 --- a/js/api/client.js +++ b/js/api/client.js @@ -41,6 +41,7 @@ let _authTokenCache = { value: "", at: 0 }; const TAGS_CACHE_TTL_MS = 30_000; const DEFAULT_TAGS_CACHE_TTL_MS = TAGS_CACHE_TTL_MS; const CLIENT_GLOBAL_KEY = "__MJR_API_CLIENT__"; +const RUNTIME_TOKEN_KEY = "__mjr_write_token"; const SETTINGS_FAST_CACHE_TTL_MS = 2000; const MAX_BATCH_ASSET_IDS = 200; const WRITE_METHODS = new Set(["POST", "PUT", "DELETE", "PATCH"]); @@ -108,10 +109,34 @@ function _readAuthToken() { if (elapsed >= 0 && elapsed < AUTH_TOKEN_CACHE_TTL_MS) { return _authTokenCache.value; } + + try { + const sessionToken = String(sessionStorage?.getItem?.(RUNTIME_TOKEN_KEY) || "").trim(); + if (sessionToken) { + _authTokenCache = { value: sessionToken, at: now }; + return sessionToken; + } + } catch {} + try { const raw = localStorage?.getItem?.(SETTINGS_KEY); const parsed = raw ? JSON.parse(raw) : null; - const token = String(parsed?.security?.apiToken || "").trim(); + const payload = parsed?.data && typeof parsed.data === "object" ? parsed.data : parsed; + const token = String(payload?.security?.apiToken || "").trim(); + if (token) { + try { + sessionStorage?.setItem?.(RUNTIME_TOKEN_KEY, token); + } catch {} + try { + const mutable = parsed && typeof parsed === "object" ? parsed : {}; + const target = mutable?.data && typeof mutable.data === "object" ? mutable.data : mutable; + if (target?.security && typeof target.security === "object") { + target.security.apiToken = ""; + localStorage?.setItem?.(SETTINGS_KEY, JSON.stringify(mutable)); + window?.dispatchEvent?.(new CustomEvent("mjr-settings-changed", { detail: { key: "security.apiToken" } })); + } + } catch {} + } _authTokenCache = { value: token, at: now }; return token; } catch { @@ -124,17 +149,18 @@ function _persistAuthToken(token) { const normalized = String(token || "").trim(); if (!normalized) return false; try { - const raw = localStorage?.getItem?.(SETTINGS_KEY); - const parsed = raw ? JSON.parse(raw) : {}; - const next = parsed && typeof parsed === "object" ? parsed : {}; - next.security = next.security && typeof next.security === "object" ? next.security : {}; - if (String(next.security.apiToken || "").trim() === normalized) { - _authTokenCache = { value: normalized, at: Date.now() }; - return true; - } - next.security.apiToken = normalized; - localStorage?.setItem?.(SETTINGS_KEY, JSON.stringify(next)); + sessionStorage?.setItem?.(RUNTIME_TOKEN_KEY, normalized); _authTokenCache = { value: normalized, at: Date.now() }; + try { + const raw = localStorage?.getItem?.(SETTINGS_KEY); + const parsed = raw ? JSON.parse(raw) : {}; + const next = parsed && typeof parsed === "object" ? parsed : {}; + const target = next?.data && typeof next.data === "object" ? next.data : next; + if (target?.security && typeof target.security === "object" && String(target.security.apiToken || "").trim()) { + target.security.apiToken = ""; + localStorage?.setItem?.(SETTINGS_KEY, JSON.stringify(next)); + } + } catch {} try { window?.dispatchEvent?.(new CustomEvent("mjr-settings-changed", { detail: { key: "security.apiToken" } })); } catch {} @@ -159,7 +185,8 @@ async function _refreshAuthTokenFromServer() { const payload = await response.json().catch(() => null); if (!payload || typeof payload !== "object" || !payload.ok) return false; const token = String(payload?.data?.token || "").trim(); - return _persistAuthToken(token); + if (token) return _persistAuthToken(token); + return true; } catch { return false; } diff --git a/js/app/comfyApiBridge.js b/js/app/comfyApiBridge.js index dfcf64c3..223c7312 100644 --- a/js/app/comfyApiBridge.js +++ b/js/app/comfyApiBridge.js @@ -11,41 +11,61 @@ function _isObject(value) { return !!value && typeof value === "object"; } +function _looksLikeComfyApi(value) { + if (!_isObject(value)) return false; + return ( + typeof value.fetchApi === "function" + || typeof value.apiURL === "function" + || _isObject(value.settings) + ); +} + +function _looksLikeComfyApp(value) { + if (!_isObject(value)) return false; + return ( + _isObject(value.ui) + || _isObject(value.canvas) + || _isObject(value.graph) + || typeof value.loadGraphData === "function" + || _looksLikeComfyApi(value.api) + ); +} + export function setComfyApp(app) { - if (app && typeof app === "object") { + if (_looksLikeComfyApp(app)) { _comfyAppRef = app; } return _comfyAppRef; } export function setComfyApi(api) { - if (api && typeof api === "object") { + if (_looksLikeComfyApi(api)) { _comfyApiRef = api; } return _comfyApiRef; } export function getComfyApi(app) { - if (_isObject(_comfyApiRef)) return _comfyApiRef; + if (_looksLikeComfyApi(_comfyApiRef)) return _comfyApiRef; const runtimeApp = _isObject(app) ? app : getComfyApp(); const fromApp = runtimeApp?.api || runtimeApp?.ui?.api || runtimeApp?.ui?.app?.api || null; - if (_isObject(fromApp)) return fromApp; + if (_looksLikeComfyApi(fromApp)) return fromApp; try { - if (typeof window !== "undefined" && _isObject(window?.api)) return window.api; + if (typeof window !== "undefined" && _looksLikeComfyApi(window?.api)) return window.api; } catch {} try { - if (typeof globalThis !== "undefined" && _isObject(globalThis?.api)) return globalThis.api; + if (typeof globalThis !== "undefined" && _looksLikeComfyApi(globalThis?.api)) return globalThis.api; } catch {} return null; } export function getComfyApp() { - if (_comfyAppRef && typeof _comfyAppRef === "object") return _comfyAppRef; + if (_looksLikeComfyApp(_comfyAppRef)) return _comfyAppRef; try { - if (typeof globalThis !== "undefined" && globalThis?.app) return globalThis.app; + if (typeof globalThis !== "undefined" && _looksLikeComfyApp(globalThis?.app)) return globalThis.app; } catch {} try { - if (typeof window !== "undefined" && window?.app) return window.app; + if (typeof window !== "undefined" && _looksLikeComfyApp(window?.app)) return window.app; } catch {} return null; } diff --git a/js/app/i18n.js b/js/app/i18n.js index 016613e4..2da75c35 100644 --- a/js/app/i18n.js +++ b/js/app/i18n.js @@ -8,7 +8,7 @@ import { SettingsStore } from "./settings/SettingsStore.js"; const DEFAULT_LANG = "en-US"; let currentLang = DEFAULT_LANG; -let _langChangeListeners = []; +const _langChangeListeners = new Set(); const LANG_STORAGE_KEYS = ["mjr_lang", "majoor.lang"]; const FOLLOW_COMFY_LANG_STORAGE_KEY = "mjr_lang_follow_comfy"; const _missingTranslationKeys = new Set(); @@ -964,11 +964,21 @@ export const setLang = (lang) => { _persistLang(lang); // Notify listeners - _langChangeListeners.forEach(cb => { + Array.from(_langChangeListeners).forEach(cb => { try { cb(lang); } catch {} }); }; +export const subscribeLangChange = (callback) => { + if (typeof callback !== "function") return () => {}; + _langChangeListeners.add(callback); + return () => { + try { + _langChangeListeners.delete(callback); + } catch {} + }; +}; + export const setFollowComfyLanguage = (enabled) => { _persistFollowComfyLang(!!enabled); }; diff --git a/js/app/settings/SettingsStore.js b/js/app/settings/SettingsStore.js index 3daa1966..5bdbcff7 100644 --- a/js/app/settings/SettingsStore.js +++ b/js/app/settings/SettingsStore.js @@ -5,6 +5,7 @@ const _subscribers = new Map(); let _storageListenerBound = false; +let _storageListener = null; function _getStorage() { try { @@ -30,11 +31,12 @@ function _emit(key, nextValue, previousValue) { function _bindStorageListener() { if (_storageListenerBound) return; try { - window.addEventListener("storage", (event) => { + _storageListener = (event) => { const key = String(event?.key || ""); if (!key) return; _emit(key, event?.newValue ?? null, event?.oldValue ?? null); - }); + }; + window.addEventListener("storage", _storageListener); _storageListenerBound = true; } catch {} } @@ -104,5 +106,16 @@ export const SettingsStore = { } catch {} return out; }, + + dispose() { + try { + if (_storageListenerBound && _storageListener && typeof window !== "undefined") { + window.removeEventListener("storage", _storageListener); + } + } catch {} + _storageListenerBound = false; + _storageListener = null; + _subscribers.clear(); + }, }; diff --git a/js/app/settings/settingsCore.js b/js/app/settings/settingsCore.js index 8290f39b..d618e443 100644 --- a/js/app/settings/settingsCore.js +++ b/js/app/settings/settingsCore.js @@ -210,9 +210,13 @@ export const loadMajoorSettings = () => { export const saveMajoorSettings = (settings) => { try { + const sanitized = JSON.parse(JSON.stringify(settings || {})); + if (sanitized?.security && typeof sanitized.security === "object") { + sanitized.security.apiToken = ""; + } const wrapped = { version: SETTINGS_SCHEMA_VERSION, - data: settings, + data: sanitized, }; SettingsStore.set(SETTINGS_KEY, JSON.stringify(wrapped)); } catch (error) { diff --git a/js/app/settings/settingsSecurity.js b/js/app/settings/settingsSecurity.js index 1ad62300..48eb570f 100644 --- a/js/app/settings/settingsSecurity.js +++ b/js/app/settings/settingsSecurity.js @@ -106,6 +106,9 @@ export function registerSecuritySettings(safeAddSetting, settings, notifyApplied settings.security.apiToken = typeof value === "string" ? value.trim() : ""; saveMajoorSettings(settings); notifyApplied("security.apiToken"); + // Skip backend call when token is empty: the token is auto-generated and + // managed server-side, so an empty field just means "use the auto-generated one". + if (!settings.security.apiToken) return; try { setSecuritySettings({ api_token: settings.security.apiToken }) .then((res) => { diff --git a/js/app/toast.js b/js/app/toast.js index f1b511c9..387d8e7b 100644 --- a/js/app/toast.js +++ b/js/app/toast.js @@ -108,9 +108,9 @@ function getToastContainer() { * Shows a toast notification. * @param {string} message The message to display * @param {'info'|'success'|'warning'|'error'} type The type of message - * @param {number} duration Duration in ms (default 3000) + * @param {number} duration Duration in ms (default 5000) */ -export function comfyToast(message, type = "info", duration = 3000) { +export function comfyToast(message, type = "info", duration = 5000) { message = translateToastMessage(message); const persistent = !(Number.isFinite(Number(duration)) && Number(duration) > 0); const app = getComfyApp(); @@ -169,7 +169,7 @@ export function comfyToast(message, type = "info", duration = 3000) { break; case "error": icon = ""; - duration = duration < 4000 ? 5000 : duration; + duration = duration < 7000 ? 8000 : duration; break; case "warning": icon = ""; diff --git a/js/app/versionCheck.js b/js/app/versionCheck.js index bc92b97b..a126d3ee 100644 --- a/js/app/versionCheck.js +++ b/js/app/versionCheck.js @@ -59,13 +59,43 @@ function isNewerVersion(remote, local) { return false; } +function createFetchTimeoutSignal(timeoutMs) { + const ms = Math.max(1, Number(timeoutMs) || 10000); + try { + if (typeof AbortSignal !== "undefined" && typeof AbortSignal.timeout === "function") { + return { signal: AbortSignal.timeout(ms), cleanup: () => {} }; + } + } catch {} + try { + if (typeof AbortController !== "undefined") { + const controller = new AbortController(); + const timer = setTimeout(() => { + try { + controller.abort(); + } catch {} + }, ms); + return { + signal: controller.signal, + cleanup: () => { + try { + clearTimeout(timer); + } catch {} + }, + }; + } + } catch {} + return { signal: undefined, cleanup: () => {} }; +} + async function fetchLatestReleaseVersion() { + const { signal, cleanup } = createFetchTimeoutSignal(10_000); const response = await fetch(LATEST_RELEASE_URL, { cache: "no-cache", + signal, headers: { Accept: "application/vnd.github+json", }, - }); + }).finally(() => cleanup()); if (!response.ok) { throw new Error(`GitHub release request failed (${response.status})`); } diff --git a/js/components/RatingEditor.js b/js/components/RatingEditor.js index 0b898559..2da1fd39 100644 --- a/js/components/RatingEditor.js +++ b/js/components/RatingEditor.js @@ -44,12 +44,20 @@ export function createRatingEditor(asset, onUpdate) { } catch {} }; + const retryDelay = (attemptIndex) => Math.min(100 * (2 ** Math.max(0, attemptIndex - 1)), 2000); + const wait = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + const flushSaves = async () => { if (saving) return; saving = true; setDisabled(true); try { - while (true) { + let attempts = 0; + while (attempts < 10) { + if (attempts > 0) { + await wait(retryDelay(attempts)); + } + attempts += 1; const toSave = Math.max(0, Math.min(5, Number(desiredRating) || 0)); const ac = typeof AbortController !== "undefined" ? new AbortController() : null; saveAC = ac; @@ -96,6 +104,13 @@ export function createRatingEditor(asset, onUpdate) { break; } } + if (attempts >= 10) { + asset.rating = savedRating; + currentRating = savedRating; + desiredRating = savedRating; + updateStarColors(container, savedRating); + comfyToast(t("toast.ratingUpdateFailed"), "error"); + } } catch {} try { diff --git a/js/components/TagsEditor.js b/js/components/TagsEditor.js index e2224d06..d9fc378b 100644 --- a/js/components/TagsEditor.js +++ b/js/components/TagsEditor.js @@ -235,6 +235,8 @@ export function createTagsEditor(asset, onUpdate) { let savePending = false; let saveAC = null; let lastSaved = Array.isArray(asset.tags) ? [...asset.tags] : []; + const retryDelay = (attemptIndex) => Math.min(100 * (2 ** Math.max(0, attemptIndex - 1)), 2000); + const wait = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); const saveTags = async () => { if (saveInFlight) { savePending = true; @@ -244,7 +246,12 @@ export function createTagsEditor(asset, onUpdate) { return; } saveInFlight = true; - while (true) { + let attempts = 0; + while (attempts < 10) { + if (attempts > 0) { + await wait(retryDelay(attempts)); + } + attempts += 1; const snapshot = [...currentTags]; const ac = typeof AbortController !== "undefined" ? new AbortController() : null; saveAC = ac; @@ -290,6 +297,12 @@ export function createTagsEditor(asset, onUpdate) { if (!savePending) break; savePending = false; } + if (attempts >= 10) { + currentTags.splice(0, currentTags.length, ...lastSaved); + asset.tags = [...lastSaved]; + renderTags(); + comfyToast(t("toast.tagsUpdateFailed"), "error"); + } saveInFlight = false; saveAC = null; }; diff --git a/js/components/Viewer_impl.js b/js/components/Viewer_impl.js index 8ca61d1c..fb9ff68a 100644 --- a/js/components/Viewer_impl.js +++ b/js/components/Viewer_impl.js @@ -36,6 +36,7 @@ import { createFrameExporter } from "../features/viewer/frameExport.js"; import { createImagePreloader } from "../features/viewer/imagePreloader.js"; import { getViewerInstance as _getViewerInstance } from "../features/viewer/viewerInstanceManager.js"; import { createPlayerBarManager } from "../features/viewer/playerBarManager.js"; +import { getHotkeysState, setHotkeysScope } from "../features/panel/controllers/hotkeysState.js"; /** * Viewer modes @@ -535,7 +536,8 @@ function createViewer() { z-index: 10001; `; const genInfoHeaderLeft = genInfoHeader.cloneNode(true); // Shallow clone structure - genInfoHeaderLeft.innerHTML = ""; // Clear content + // safe: clearing existing static node content only + genInfoHeaderLeft.innerHTML = ""; const genInfoTitleLeft = document.createElement("div"); genInfoTitleLeft.textContent = "Generation Info (A)"; genInfoTitleLeft.style.cssText = "font-size: 13px; font-weight: 600;"; @@ -1961,10 +1963,8 @@ function createViewer() { } catch {} // Restore previous hotkeys scope if available. - if (window._mjrHotkeysState) { - const prevScope = state?._prevHotkeyScope; - window._mjrHotkeysState.scope = prevScope || "panel"; - } + const prevScope = state?._prevHotkeyScope; + setHotkeysScope(prevScope || "panel"); state._prevHotkeyScope = null; } @@ -2019,9 +2019,8 @@ function createViewer() { document.body.style.overflow = 'hidden'; // Set hotkeys scope to viewer (takes priority over panel) - if (!window._mjrHotkeysState) window._mjrHotkeysState = {}; - state._prevHotkeyScope = window._mjrHotkeysState.scope || null; - window._mjrHotkeysState.scope = "viewer"; + state._prevHotkeyScope = getHotkeysState().scope || null; + setHotkeysScope("viewer"); updateUI(); try { diff --git a/js/components/sidebar/sections/GenerationSection.js b/js/components/sidebar/sections/GenerationSection.js index 81a1dac1..97901662 100644 --- a/js/components/sidebar/sections/GenerationSection.js +++ b/js/components/sidebar/sections/GenerationSection.js @@ -37,6 +37,18 @@ function _setPreviewSrcWithFallback(el, candidates) { apply(); } +function _isSafeOpenUrl(url) { + const value = String(url || "").trim(); + if (!value) return false; + if (value.startsWith("/")) return true; + try { + const parsed = new URL(value); + return parsed.protocol === "http:" || parsed.protocol === "https:"; + } catch { + return false; + } +} + export function createGenerationSection(asset) { let metadata = null; @@ -534,7 +546,9 @@ export function createGenerationSection(asset) { }; thumb.ondblclick = (e) => { e.stopPropagation(); - window.open(src, "_blank"); + const src = Array.isArray(srcCandidates) && srcCandidates.length ? String(srcCandidates[0] || "") : ""; + if (!_isSafeOpenUrl(src)) return; + window.open(src, "_blank", "noopener,noreferrer"); }; grid.appendChild(thumb); diff --git a/js/components/sidebar/sections/PreviewSection.js b/js/components/sidebar/sections/PreviewSection.js index 144d56c8..6c88f806 100644 --- a/js/components/sidebar/sections/PreviewSection.js +++ b/js/components/sidebar/sections/PreviewSection.js @@ -36,9 +36,9 @@ export function createPreviewSection(asset, options = {}) { const video = document.createElement("video"); video.src = viewUrl; video.controls = false; + video.muted = true; video.autoplay = true; video.loop = true; - video.muted = true; video.playsInline = true; video.preload = "metadata"; video.style.cssText = ` diff --git a/js/components/sidebar/utils/dom.js b/js/components/sidebar/utils/dom.js index edfd5b47..5a8278e8 100644 --- a/js/components/sidebar/utils/dom.js +++ b/js/components/sidebar/utils/dom.js @@ -2,6 +2,14 @@ import { comfyToast } from "../../../app/toast.js"; import { t } from "../../../app/i18n.js"; import { copyTextToClipboard } from "../../../utils/dom.js"; +const _ALLOWED_VALUE_STYLE_PROPS = new Set([ + "color", + "font-weight", + "font-style", + "opacity", + "text-decoration", +]); + export function createSection(title) { const section = document.createElement("div"); section.className = "mjr-sidebar-section"; @@ -80,6 +88,7 @@ export function createInfoBox(title, content, accentColor, options = {}) { width: 16px; height: 16px; `; + // safe: static SVG literal, no user-controlled interpolation copyBtn.innerHTML = ` @@ -92,6 +101,7 @@ export function createInfoBox(title, content, accentColor, options = {}) { e.stopPropagation(); const originalHTML = copyBtn.innerHTML; await doCopy(); + // safe: static SVG literal, no user-controlled interpolation copyBtn.innerHTML = ` @@ -192,8 +202,8 @@ export function createParametersBox(title, fields, accentColor, options = {}) { word-break: break-word; white-space: pre-wrap; cursor: pointer; - ${field.valueStyle || ""} `; + _applySafeValueStyle(value, field?.valueStyle); value.addEventListener("click", async () => { const ok = await copyTextToClipboard(String(field.value)); @@ -211,6 +221,23 @@ export function createParametersBox(title, fields, accentColor, options = {}) { return box; } +function _applySafeValueStyle(el, rawStyle) { + if (!el || !rawStyle) return; + const text = String(rawStyle || ""); + const declarations = text.split(";"); + for (const decl of declarations) { + const idx = decl.indexOf(":"); + if (idx <= 0) continue; + const prop = decl.slice(0, idx).trim().toLowerCase(); + const val = decl.slice(idx + 1).trim(); + if (!prop || !val) continue; + if (!_ALLOWED_VALUE_STYLE_PROPS.has(prop)) continue; + try { + el.style.setProperty(prop, val); + } catch {} + } +} + function hexToRgba(hex, alpha) { const normalized = String(hex || "").trim().replace(/^#/, ""); if (!/^[0-9a-fA-F]{6}$/.test(normalized)) return `rgba(255,255,255,${alpha})`; diff --git a/js/features/collections/controllers/collectionsController.js b/js/features/collections/controllers/collectionsController.js index f620e503..ae03ea76 100644 --- a/js/features/collections/controllers/collectionsController.js +++ b/js/features/collections/controllers/collectionsController.js @@ -3,6 +3,24 @@ import { comfyToast } from "../../../app/toast.js"; import { listCollections, createCollection, deleteCollection } from "../../../api/client.js"; import { t } from "../../../app/i18n.js"; +const MAX_COLLECTION_NAME_LEN = 128; + +function normalizeCollectionName(value) { + try { + const name = String(value ?? "").trim(); + if (!name) return ""; + if (name.length > MAX_COLLECTION_NAME_LEN) return ""; + if (name.includes("\x00")) return ""; + for (let i = 0; i < name.length; i += 1) { + const code = name.charCodeAt(i); + if (code <= 31 || code === 127) return ""; + } + return name; + } catch { + return ""; + } +} + function createMenuItem(label, { right = null, checked = false, danger = false } = {}) { const btn = document.createElement("button"); btn.type = "button"; @@ -75,13 +93,18 @@ export function createCollectionsController({ state, collectionsBtn, collections createBtn.addEventListener("click", async () => { const name = await comfyPrompt(t("dialog.createCollection"), t("dialog.collectionPlaceholder")); if (!name) return; - const res = await createCollection(name); + const normalizedName = normalizeCollectionName(name); + if (!normalizedName) { + comfyToast(t("toast.invalidCollectionName", "Invalid collection name"), "error"); + return; + } + const res = await createCollection(normalizedName); if (!res?.ok) { comfyToast(res?.error || t("toast.failedCreateCollection"), "error"); return; } state.collectionId = String(res.data?.id || ""); - state.collectionName = String(res.data?.name || name); + state.collectionName = String(res.data?.name || normalizedName); try { onChanged?.(); } catch {} diff --git a/js/features/dnd/DragDrop.js b/js/features/dnd/DragDrop.js index dd81430d..58ffaa00 100644 --- a/js/features/dnd/DragDrop.js +++ b/js/features/dnd/DragDrop.js @@ -35,6 +35,66 @@ const buildURL = (payload) => const _workflowCache = new Map(); const WORKFLOW_CACHE_TTL_MS = 60_000; // 1 minute const WORKFLOW_CACHE_MAX = 100; +const MAX_WORKFLOW_BYTES = 5 * 1024 * 1024; +const MAX_WORKFLOW_NODE_COUNT = 5000; +const MAX_WORKFLOW_LINK_COUNT = 20000; +const MAX_WORKFLOW_NODE_TYPE_LENGTH = 256; +const MAX_WORKFLOW_WIDGET_STRING_LENGTH = 8192; +const _NODE_TYPE_CTRL_RE = /[\u0000-\u001f\u007f]/; +const _NUL_RE = /\u0000/; + +const _hasNodeTypeControlChars = (value) => _NODE_TYPE_CTRL_RE.test(String(value || "")); +const _hasNulByte = (value) => _NUL_RE.test(String(value || "")); + +const _isSafeWorkflowNode = (node) => { + if (!node || typeof node !== "object" || Array.isArray(node)) return false; + const id = Number(node.id); + if (!Number.isFinite(id)) return false; + + const nodeType = node.type == null ? "" : String(node.type); + if (nodeType) { + if (nodeType.length > MAX_WORKFLOW_NODE_TYPE_LENGTH) return false; + if (_hasNodeTypeControlChars(nodeType)) return false; + } + + const widgetsValues = node.widgets_values; + if (Array.isArray(widgetsValues)) { + for (const value of widgetsValues) { + if (typeof value === "string") { + if (value.length > MAX_WORKFLOW_WIDGET_STRING_LENGTH) return false; + if (_hasNulByte(value)) return false; + } + } + } + + return true; +}; + +const _isSafeWorkflowLink = (link) => { + if (Array.isArray(link)) return link.length >= 4; + if (link && typeof link === "object") { + const id = Number(link.id); + if (!Number.isFinite(id)) return false; + return true; + } + return false; +}; + +const isValidWorkflowShape = (workflow) => { + if (!workflow || typeof workflow !== "object") return false; + if (!Array.isArray(workflow.nodes) || !Array.isArray(workflow.links)) return false; + if (workflow.nodes.length > MAX_WORKFLOW_NODE_COUNT) return false; + if (workflow.links.length > MAX_WORKFLOW_LINK_COUNT) return false; + if (!workflow.nodes.every(_isSafeWorkflowNode)) return false; + if (!workflow.links.every(_isSafeWorkflowLink)) return false; + try { + const size = JSON.stringify(workflow).length; + if (!Number.isFinite(size) || size <= 0 || size > MAX_WORKFLOW_BYTES) return false; + } catch { + return false; + } + return true; +}; const cleanupWorkflowCache = () => { if (_workflowCache.size <= WORKFLOW_CACHE_MAX) return; @@ -71,7 +131,7 @@ const tryLoadWorkflowToCanvas = async (payload, fallbackAbsPath = null) => { const now = Date.now(); if (cached && now - (cached.at || 0) < WORKFLOW_CACHE_TTL_MS) { - if (cached.workflow) { + if (cached.workflow && isValidWorkflowShape(cached.workflow)) { try { if (typeof app?.loadGraphData === "function") { app.loadGraphData(cached.workflow); @@ -129,22 +189,12 @@ const tryLoadWorkflowToCanvas = async (payload, fallbackAbsPath = null) => { if (!workflow && url) { const res = await get(url); if (!res?.ok || !res.data) { - // Cache negative result to avoid retrying - if (cacheKey) { - _workflowCache.set(cacheKey, { workflow: null, at: Date.now() }); - cleanupWorkflowCache(); - } return false; } workflow = res.data?.workflow; } - if (!workflow || typeof workflow !== "object") { - // Cache negative result - if (cacheKey) { - _workflowCache.set(cacheKey, { workflow: null, at: Date.now() }); - cleanupWorkflowCache(); - } + if (!isValidWorkflowShape(workflow)) { return false; } diff --git a/js/features/dnd/targets/node.js b/js/features/dnd/targets/node.js index 2f44d934..5cb92049 100644 --- a/js/features/dnd/targets/node.js +++ b/js/features/dnd/targets/node.js @@ -5,8 +5,19 @@ import { looksLikeVideoPath } from "../utils/video.js"; -let highlightedNode = null; -let highlightedNodePrev = null; +const highlightState = new WeakMap(); + +const getHighlightState = (app) => { + if (!app || typeof app !== "object") { + return { node: null, prev: null }; + } + let state = highlightState.get(app); + if (!state) { + state = { node: null, prev: null }; + highlightState.set(app, state); + } + return state; +}; export const getNodeUnderClientXY = (app, clientX, clientY) => { const canvasEl = app?.canvas?.canvas || document.querySelector("canvas"); @@ -52,25 +63,27 @@ export const getNodeUnderClientXY = (app, clientX, clientY) => { }; export const applyHighlight = (app, node, markCanvasDirty) => { - if (!node || highlightedNode === node) return; + const state = getHighlightState(app); + if (!node || state.node === node) return; clearHighlight(app, markCanvasDirty); - highlightedNode = node; - highlightedNodePrev = { color: node.color, bgcolor: node.bgcolor }; + state.node = node; + state.prev = { color: node.color, bgcolor: node.bgcolor }; node.bgcolor = "#3355ff"; node.color = "#a9c4ff"; markCanvasDirty(app); }; export const clearHighlight = (app, markCanvasDirty) => { - if (!highlightedNode) return; + const state = getHighlightState(app); + if (!state.node) return; try { - if (highlightedNodePrev) { - highlightedNode.color = highlightedNodePrev.color; - highlightedNode.bgcolor = highlightedNodePrev.bgcolor; + if (state.prev) { + state.node.color = state.prev.color; + state.node.bgcolor = state.prev.bgcolor; } } catch {} - highlightedNode = null; - highlightedNodePrev = null; + state.node = null; + state.prev = null; markCanvasDirty(app); }; diff --git a/js/features/grid/GridKeyboard.js b/js/features/grid/GridKeyboard.js index 1e478a9f..465558d9 100644 --- a/js/features/grid/GridKeyboard.js +++ b/js/features/grid/GridKeyboard.js @@ -19,6 +19,7 @@ import { safeDispatchCustomEvent } from "../../utils/events.js"; import { getViewerInstance } from "../../components/Viewer.js"; import { showAddToCollectionMenu } from "../collections/contextmenu/addToCollectionMenu.js"; import { normalizeRenameFilename, validateFilename } from "../../utils/filenames.js"; +import { getHotkeysState, isHotkeysSuspended } from "../panel/controllers/hotkeysState.js"; /** * Keyboard shortcut definitions @@ -352,9 +353,9 @@ export function installGridKeyboard({ } // Skip if hotkeys suspended (dialog open, etc.) - if (globalThis?._mjrHotkeysState?.suspended) return; + if (isHotkeysSuspended()) return; // Viewer must have priority over grid shortcuts. - if (globalThis?._mjrHotkeysState?.scope === "viewer") return; + if (getHotkeysState().scope === "viewer") return; // Skip if grid not visible/focused // Allow if focus is: @@ -391,7 +392,7 @@ export function installGridKeyboard({ const state = getState(); // Rating shortcuts (1-5, 0 for reset) - handled by dedicated rating controller when enabled. - const ratingHotkeysActive = !!globalThis?._mjrRatingHotkeysActive; + const ratingHotkeysActive = !!getHotkeysState().ratingHotkeysActive; if (!e.ctrlKey && !e.altKey && !e.metaKey && !e.shiftKey) { if (!ratingHotkeysActive && matchesShortcut(e, GRID_SHORTCUTS.RATING_1)) { if (asset?.id) { consume(); setRating(asset, 1, onAssetChanged); return; } @@ -749,6 +750,10 @@ export function installGridKeyboard({ document.removeEventListener("keydown", keydownHandler, true); keydownHandler = null; bound = false; + for (const t of _ratingDebounceTimers.values()) { + try { clearTimeout(t); } catch {} + } + _ratingDebounceTimers.clear(); }; const dispose = () => { diff --git a/js/features/panel/AssetsManagerPanel.js b/js/features/panel/AssetsManagerPanel.js index 11b3909a..755ce0f4 100644 --- a/js/features/panel/AssetsManagerPanel.js +++ b/js/features/panel/AssetsManagerPanel.js @@ -55,6 +55,7 @@ import { createSortController } from "./controllers/sortController.js"; import { bindSidebarOpen } from "./controllers/sidebarController.js"; import { createPanelHotkeysController } from "./controllers/panelHotkeysController.js"; import { createRatingHotkeysController } from "./controllers/ratingHotkeysController.js"; +import { getHotkeysState, setHotkeysScope } from "./controllers/hotkeysState.js"; import { bindGridContextMenu } from "../contextmenu/GridContextMenu.js"; import { getViewerInstance } from "../../components/Viewer.js"; @@ -424,7 +425,7 @@ export async function renderAssetsManager(container, { useComfyThemeUI = true } try { container._mjrVersionUpdateCleanup?.(); } catch {} - container._mjrVersionUpdateCleanup = header._mjrVersionUpdateCleanup; + container._mjrVersionUpdateCleanup = headerView.dispose || header._mjrVersionUpdateCleanup; container.appendChild(content); const getQuery = () => normalizeQuery(searchInputEl); @@ -1074,10 +1075,7 @@ export async function renderAssetsManager(container, { useComfyThemeUI = true } hotkeys.bind(content); ratingHotkeys.bind(); - try { - window._mjrHotkeysState = window._mjrHotkeysState || {}; - window._mjrHotkeysState.scope = "grid"; - } catch {} + setHotkeysScope("grid"); // Attach controllers to container for cleanup on re-render try { @@ -1113,11 +1111,9 @@ export async function renderAssetsManager(container, { useComfyThemeUI = true } try { ratingHotkeys.dispose(); } catch {} - try { - if (window._mjrHotkeysState?.scope === "grid") { - window._mjrHotkeysState.scope = null; - } - } catch {} + if (getHotkeysState().scope === "grid") { + setHotkeysScope(null); + } try { agendaCalendar?.dispose?.(); } catch {} diff --git a/js/features/panel/controllers/contextController.js b/js/features/panel/controllers/contextController.js index 3074764a..6b77a6ac 100644 --- a/js/features/panel/controllers/contextController.js +++ b/js/features/panel/controllers/contextController.js @@ -275,9 +275,11 @@ export function createContextController({ } }; if (extraActions && typeof extraActions === "object") { - try { - Object.assign(actions, extraActions); - } catch {} + const blocked = new Set(["__proto__", "constructor", "prototype"]); + for (const [key, value] of Object.entries(extraActions)) { + if (blocked.has(String(key))) continue; + actions[key] = value; + } } const update = () => { diff --git a/js/features/panel/controllers/filtersController.js b/js/features/panel/controllers/filtersController.js index 55a3ce10..c6819ea1 100644 --- a/js/features/panel/controllers/filtersController.js +++ b/js/features/panel/controllers/filtersController.js @@ -1,5 +1,3 @@ -let _lastAgendaHandler = null; - const parseLooseNumber = (value) => { const raw = String(value ?? "").trim(); if (!raw) return 0; @@ -38,7 +36,6 @@ export function bindFilters({ return; } } - if (lifecycleSignal) return; disposers.push(() => { try { target.removeEventListener(event, handler, options); @@ -50,14 +47,6 @@ export function bindFilters({ }); }; - // Cleanup previous window listener to prevent accumulation - if (_lastAgendaHandler) { - try { - window.removeEventListener("MJR:AgendaStatus", _lastAgendaHandler); - } catch {} - _lastAgendaHandler = null; - } - addManagedListener(kindSelect, "change", async () => { state.kindFilter = kindSelect.value || ""; try { onFiltersChanged?.(); } catch {} @@ -209,18 +198,11 @@ export function bindFilters({ try { const opts = lifecycleSignal ? { passive: true, signal: lifecycleSignal } : { passive: true }; window?.addEventListener?.("MJR:AgendaStatus", handleAgendaStatus, opts); - _lastAgendaHandler = handleAgendaStatus; - if (!lifecycleSignal) { - disposers.push(() => { - try { - window.removeEventListener("MJR:AgendaStatus", handleAgendaStatus); - } catch {} - }); - } else { - lifecycleSignal.addEventListener("abort", () => { - if (_lastAgendaHandler === handleAgendaStatus) _lastAgendaHandler = null; - }, { once: true }); - } + disposers.push(() => { + try { + window.removeEventListener("MJR:AgendaStatus", handleAgendaStatus); + } catch {} + }); } catch {} } @@ -230,12 +212,6 @@ export function bindFilters({ dispose(); } catch {} } - if (_lastAgendaHandler) { - try { - window.removeEventListener("MJR:AgendaStatus", _lastAgendaHandler); - } catch {} - _lastAgendaHandler = null; - } }; } diff --git a/js/features/panel/controllers/hotkeysState.js b/js/features/panel/controllers/hotkeysState.js new file mode 100644 index 00000000..da0e393b --- /dev/null +++ b/js/features/panel/controllers/hotkeysState.js @@ -0,0 +1,25 @@ +const HOTKEYS_STATE = { + suspended: false, + scope: null, + ratingHotkeysActive: false, +}; + +export function getHotkeysState() { + return HOTKEYS_STATE; +} + +export function setHotkeysScope(scope) { + HOTKEYS_STATE.scope = scope == null ? null : String(scope); +} + +export function isHotkeysSuspended() { + return !!HOTKEYS_STATE.suspended; +} + +export function setHotkeysSuspendedFlag(suspended) { + HOTKEYS_STATE.suspended = Boolean(suspended); +} + +export function setRatingHotkeysActive(active) { + HOTKEYS_STATE.ratingHotkeysActive = Boolean(active); +} diff --git a/js/features/panel/controllers/panelHotkeysController.js b/js/features/panel/controllers/panelHotkeysController.js index 9d6292ae..30d8a8a4 100644 --- a/js/features/panel/controllers/panelHotkeysController.js +++ b/js/features/panel/controllers/panelHotkeysController.js @@ -1,17 +1,12 @@ -// Lazily initialize shared hotkeys state to avoid module-load side effects. -function ensureHotkeysState() { - const root = typeof globalThis !== "undefined" ? globalThis : null; - if (!root) return null; - if (!root._mjrHotkeysState || typeof root._mjrHotkeysState !== "object") { - root._mjrHotkeysState = { suspended: false }; - } - return root._mjrHotkeysState; -} +import { + getHotkeysState, + isHotkeysSuspended, + setHotkeysScope, + setHotkeysSuspendedFlag, +} from "./hotkeysState.js"; export function setHotkeysSuspended(suspended) { - const state = ensureHotkeysState(); - if (!state) return; - state.suspended = Boolean(suspended); + setHotkeysSuspendedFlag(suspended); } export function createPanelHotkeysController({ @@ -22,7 +17,6 @@ export function createPanelHotkeysController({ onClearSearch, allowListKeys, } = {}) { - ensureHotkeysState(); let panelFocused = false; let panelHovered = false; let boundEl = null; @@ -62,10 +56,10 @@ export function createPanelHotkeysController({ }, keydown: (event) => { // Check if hotkeys are suspended (e.g. when dialogs/popovers are open) - if (window._mjrHotkeysState?.suspended) return; + if (isHotkeysSuspended()) return; // Check if viewer has hotkey priority - if (window._mjrHotkeysState?.scope === "viewer") return; + if (getHotkeysState().scope === "viewer") return; // Check if panel is active before processing hotkeys // This ensures hotkeys only work when the panel is hovered or focused @@ -151,6 +145,7 @@ export function createPanelHotkeysController({ try { window.addEventListener("keydown", handlers.keydown, { capture: true }); } catch {} + setHotkeysScope("panel"); }; const dispose = () => { @@ -166,6 +161,9 @@ export function createPanelHotkeysController({ try { window.removeEventListener("keydown", handlers.keydown, { capture: true }); } catch {} + if (getHotkeysState().scope === "panel") { + setHotkeysScope(null); + } boundEl = null; }; diff --git a/js/features/panel/controllers/ratingHotkeysController.js b/js/features/panel/controllers/ratingHotkeysController.js index 404d4298..08b78eca 100644 --- a/js/features/panel/controllers/ratingHotkeysController.js +++ b/js/features/panel/controllers/ratingHotkeysController.js @@ -2,6 +2,7 @@ import { updateAssetRating } from "../../../api/client.js"; import { ASSET_RATING_CHANGED_EVENT } from "../../../app/events.js"; import { comfyToast } from "../../../app/toast.js"; import { t } from "../../../app/i18n.js"; +import { getHotkeysState, isHotkeysSuspended, setRatingHotkeysActive } from "./hotkeysState.js"; const EVENT_NAME = ASSET_RATING_CHANGED_EVENT; @@ -41,6 +42,25 @@ function updateCardRatingBadge(card, createRatingBadge, rating) { } catch {} } +async function runWithConcurrencyLimit(items, worker, limit = 5) { + const max = Math.max(1, Number(limit) || 1); + let index = 0; + const runners = []; + const runOne = async () => { + while (index < items.length) { + const cur = index; + index += 1; + try { + await worker(items[cur], cur); + } catch {} + } + }; + for (let i = 0; i < Math.min(max, items.length); i += 1) { + runners.push(runOne()); + } + await Promise.all(runners); +} + export function createRatingHotkeysController({ gridContainer, createRatingBadge } = {}) { let bound = false; let onKeyDown = null; @@ -69,14 +89,14 @@ export function createRatingHotkeysController({ gridContainer, createRatingBadge const bind = () => { if (bound || !gridContainer) return; bound = true; - try { window._mjrRatingHotkeysActive = true; } catch {} + setRatingHotkeysActive(true); onKeyDown = async (e) => { // Check if hotkeys are suspended (e.g. when dialogs/popovers are open) - if (window._mjrHotkeysState?.suspended) return; + if (isHotkeysSuspended()) return; // Check if viewer has hotkey priority - if (window._mjrHotkeysState?.scope === "viewer") return; + if (getHotkeysState().scope === "viewer") return; if (e.defaultPrevented) return; const k = String(e.key || ""); @@ -105,10 +125,7 @@ export function createRatingHotkeysController({ gridContainer, createRatingBadge e.stopImmediatePropagation?.(); } - // Apply sequentially to avoid hammering the backend on large multi-select. - for (const card of cards) { - await applyRatingToCard(card, rating); - } + await runWithConcurrencyLimit(cards, (card) => applyRatingToCard(card, rating), 5); }; onRatingEvent = (ev) => { @@ -136,7 +153,7 @@ export function createRatingHotkeysController({ gridContainer, createRatingBadge const dispose = () => { if (!bound) return; bound = false; - try { window._mjrRatingHotkeysActive = false; } catch {} + setRatingHotkeysActive(false); try { // Remove window-level listener window.removeEventListener("keydown", onKeyDown, { capture: true }); diff --git a/js/features/panel/state/panelState.js b/js/features/panel/state/panelState.js index 4b980260..a191dc12 100644 --- a/js/features/panel/state/panelState.js +++ b/js/features/panel/state/panelState.js @@ -117,6 +117,7 @@ export function createPanelState() { // Sidebar open state for restoring across panel close/reopen. sidebarOpen: saved.sidebarOpen || false }; + let proxy = null; let debounceTimer = null; const debouncedSave = (targetState) => { @@ -133,7 +134,10 @@ export function createPanelState() { if (!event.newValue) return; try { const updated = sanitizePanelState(JSON.parse(event.newValue)); - Object.assign(state, updated || {}); + const target = proxy || state; + for (const [k, v] of Object.entries(updated || {})) { + target[k] = v; + } } catch {} }; try { @@ -150,7 +154,7 @@ export function createPanelState() { } catch {} }; - const proxy = new Proxy(state, { + proxy = new Proxy(state, { set(target, prop, value) { target[prop] = value; debouncedSave(target); diff --git a/js/features/panel/views/contextPillsView.js b/js/features/panel/views/contextPillsView.js index f9d5af6e..b82f4135 100644 --- a/js/features/panel/views/contextPillsView.js +++ b/js/features/panel/views/contextPillsView.js @@ -87,7 +87,9 @@ export function createContextPillsView() { const anyContext = isQueryActive || isFilterActive || isSortActive || isCollectionActive || isScopeActive; - void anyContext; + if (!anyContext && !root.childElementCount) { + return; + } try { root.replaceChildren(); diff --git a/js/features/panel/views/headerView.js b/js/features/panel/views/headerView.js index 88c2feda..86bd2b9d 100644 --- a/js/features/panel/views/headerView.js +++ b/js/features/panel/views/headerView.js @@ -13,7 +13,10 @@ let _extensionMetadataPromise = null; function getExtensionMetadata() { if (!_extensionMetadataPromise) { // Keep API shape but avoid requesting a manifest file that may not exist. - _extensionMetadataPromise = Promise.resolve({}); + _extensionMetadataPromise = Promise.resolve({}).catch((err) => { + _extensionMetadataPromise = null; + throw err; + }); } return _extensionMetadataPromise; } @@ -26,7 +29,9 @@ function applyExtensionMetadata(badge, isNightly) { badge.textContent = `v${version}`; } } - }).catch(() => {}); + }).catch(() => { + _extensionMetadataPromise = null; + }); } async function hydrateBackendVersionBadge(badge, isNightly) { @@ -177,6 +182,14 @@ export function createHeaderView() { applyDotState(getStoredVersionUpdateState()); } catch {} let versionUpdateListener = null; + const dispose = () => { + try { + if (versionUpdateListener && typeof window !== "undefined") { + window.removeEventListener(VERSION_UPDATE_EVENT, versionUpdateListener); + } + } catch {} + versionUpdateListener = null; + }; if (typeof window !== "undefined") { versionUpdateListener = (event) => { try { @@ -185,13 +198,7 @@ export function createHeaderView() { }; window.addEventListener(VERSION_UPDATE_EVENT, versionUpdateListener); } - header._mjrVersionUpdateCleanup = () => { - try { - if (versionUpdateListener && typeof window !== "undefined") { - window.removeEventListener(VERSION_UPDATE_EVENT, versionUpdateListener); - } - } catch {} - }; + header._mjrVersionUpdateCleanup = dispose; applyExtensionMetadata(versionBadge, isNightly); void hydrateBackendVersionBadge(versionBadge, isNightly); @@ -206,7 +213,8 @@ export function createHeaderView() { filterBtn, sortBtn, collectionsBtn, - pinnedFoldersBtn + pinnedFoldersBtn, + dispose, }; } diff --git a/js/features/panel/views/popoverManager.js b/js/features/panel/views/popoverManager.js index a9cfb855..ac51dc38 100644 --- a/js/features/panel/views/popoverManager.js +++ b/js/features/panel/views/popoverManager.js @@ -7,6 +7,7 @@ export function createPopoverManager(container) { let windowScrollHandlerBound = false; let windowResizeHandlerBound = false; let repositionController = new AbortController(); + let disposed = false; const POPOVER_GAP_PX = 8; const POPOVER_PAD_PX = 8; @@ -159,7 +160,7 @@ export function createPopoverManager(container) { try { repositionController.abort(); } catch {} - repositionController = new AbortController(); + if (!disposed) repositionController = new AbortController(); }; const close = (popover) => { @@ -231,6 +232,7 @@ export function createPopoverManager(container) { }; const dispose = () => { + disposed = true; try { document.removeEventListener("mousedown", onDocMouseDown); } catch {} @@ -240,7 +242,7 @@ export function createPopoverManager(container) { try { repositionController.abort(); } catch {} - repositionController = new AbortController(); + repositionController = null; try { closeAll(); } catch {} diff --git a/js/features/panel/views/searchView.js b/js/features/panel/views/searchView.js index 30f8244b..5f53e56c 100644 --- a/js/features/panel/views/searchView.js +++ b/js/features/panel/views/searchView.js @@ -16,7 +16,14 @@ export function createSearchView({ filterBtn, sortBtn, collectionsBtn, pinnedFol searchIcon.appendChild(searchIconEl); // Autocomplete datalist - const dataListId = "mjr-search-autocomplete-" + Math.random().toString(36).substr(2, 9); + const dataListId = (() => { + try { + if (typeof crypto !== "undefined" && typeof crypto.randomUUID === "function") { + return `mjr-search-autocomplete-${crypto.randomUUID()}`; + } + } catch {} + return `mjr-search-autocomplete-${Math.random().toString(36).slice(2, 11)}`; + })(); const dataList = document.createElement("datalist"); dataList.id = dataListId; diff --git a/js/features/panel/views/summaryBarView.js b/js/features/panel/views/summaryBarView.js index 42ca25ff..95224da5 100644 --- a/js/features/panel/views/summaryBarView.js +++ b/js/features/panel/views/summaryBarView.js @@ -46,9 +46,16 @@ export function createSummaryBarView() { bar.appendChild(right); const update = ({ state, gridContainer, context = null, actions = null } = {}) => { + const cards = (() => { + try { + return Array.from(gridContainer?.querySelectorAll?.(".mjr-asset-card") || []); + } catch { + return []; + } + })(); + const folderStats = (() => { try { - const cards = Array.from(gridContainer?.querySelectorAll?.(".mjr-asset-card") || []); let folders = 0; for (const card of cards) { const kind = String(card?._mjrAsset?.kind || "").toLowerCase(); @@ -61,11 +68,7 @@ export function createSummaryBarView() { })(); const cardsCount = (() => { - try { - return Number(gridContainer?.querySelectorAll?.(".mjr-asset-card")?.length || 0) || 0; - } catch { - return 0; - } + return Number(cards.length || 0) || 0; })(); const selectedCount = (() => { @@ -74,7 +77,11 @@ export function createSummaryBarView() { if (fromState > 0) return fromState; } catch {} try { - return Number(gridContainer?.querySelectorAll?.(".mjr-asset-card.is-selected")?.length || 0) || 0; + let selected = 0; + for (const card of cards) { + if (card?.classList?.contains?.("is-selected")) selected += 1; + } + return selected; } catch { return 0; } diff --git a/js/features/viewer/genInfo.js b/js/features/viewer/genInfo.js index 0a907e50..c5c9e3ea 100644 --- a/js/features/viewer/genInfo.js +++ b/js/features/viewer/genInfo.js @@ -362,7 +362,14 @@ export async function ensureViewerMetadataAsset( }; if (cacheKey) { + const onAbort = () => { + try { GENINFO_INFLIGHT.delete(cacheKey); } catch {} + }; + try { + signal?.addEventListener?.("abort", onAbort, { once: true }); + } catch {} const p = run().finally(() => { + try { signal?.removeEventListener?.("abort", onAbort); } catch {} try { GENINFO_INFLIGHT.delete(cacheKey); } catch {} }); GENINFO_INFLIGHT.set(cacheKey, p); diff --git a/js/features/viewer/keyboard.js b/js/features/viewer/keyboard.js index 9dd73649..7d7f07f7 100644 --- a/js/features/viewer/keyboard.js +++ b/js/features/viewer/keyboard.js @@ -1,6 +1,7 @@ import { safeAddListener, safeCall } from "./lifecycle.js"; import { comfyToast } from "../../app/toast.js"; import { t } from "../../app/i18n.js"; +import { isHotkeysSuspended } from "../panel/controllers/hotkeysState.js"; export function installViewerKeyboard({ overlay, @@ -86,7 +87,7 @@ export function installViewerKeyboard({ }; // Check if hotkeys are suspended (e.g. when dialogs/popovers are open) - if (globalThis?._mjrHotkeysState?.suspended) return; + if (isHotkeysSuspended()) return; try { if (overlay?.style?.display === "none") return; diff --git a/js/features/viewer/mediaFactory.js b/js/features/viewer/mediaFactory.js index 8805a3ee..daceb514 100644 --- a/js/features/viewer/mediaFactory.js +++ b/js/features/viewer/mediaFactory.js @@ -364,11 +364,11 @@ export function createViewerMediaFactory({ video.className = "mjr-viewer-video-src"; video.src = url; video.controls = false; - video.autoplay = true; // Default: loop videos in the viewer. If custom controls are mounted, they may disable native looping. video.loop = true; video.playsInline = true; video.muted = true; + video.autoplay = true; video.preload = "metadata"; video.style.cssText = "position:absolute; width:1px; height:1px; opacity:0; pointer-events:none;"; _attachFpsDetection(video, asset); diff --git a/js/features/viewer/metadata.js b/js/features/viewer/metadata.js index 2651c08f..3e673151 100644 --- a/js/features/viewer/metadata.js +++ b/js/features/viewer/metadata.js @@ -170,7 +170,6 @@ export function createViewerMetadataHydrator({ safeCall(() => cache.clear()); }, cleanupCache: () => safeCall(cleanupCache) || undefined, - _unsafeCache: cache, _noop: noop, }; } diff --git a/js/tests/grid_selection_manager.test.mjs b/js/tests/grid_selection_manager.test.mjs deleted file mode 100644 index 89e89442..00000000 --- a/js/tests/grid_selection_manager.test.mjs +++ /dev/null @@ -1,74 +0,0 @@ -import test from "node:test"; -import assert from "node:assert/strict"; - -import { - getSelectedIdSet, - safeEscapeId, - setSelectionIds, - syncSelectionClasses, -} from "../features/grid/GridSelectionManager.js"; - -function makeCard(id) { - const classSet = new Set(); - const attrs = new Map(); - return { - dataset: { mjrAssetId: String(id) }, - classList: { - add(v) { - classSet.add(v); - }, - remove(v) { - classSet.delete(v); - }, - has(v) { - return classSet.has(v); - }, - }, - setAttribute(k, v) { - attrs.set(k, v); - }, - getAttribute(k) { - return attrs.get(k); - }, - }; -} - -test("getSelectedIdSet supports single and multi dataset keys", () => { - const g1 = { dataset: { mjrSelectedAssetIds: JSON.stringify([1, "2"]) } }; - assert.deepEqual(Array.from(getSelectedIdSet(g1)).sort(), ["1", "2"]); - - const g2 = { dataset: { mjrSelectedAssetId: "9" } }; - assert.deepEqual(Array.from(getSelectedIdSet(g2)), ["9"]); -}); - -test("syncSelectionClasses and setSelectionIds update cards and emit event", () => { - const cards = [makeCard(1), makeCard(2), makeCard(3)]; - const events = []; - globalThis.CustomEvent = class { - constructor(type, init) { - this.type = type; - this.detail = init?.detail; - } - }; - const grid = { - dataset: {}, - dispatchEvent(evt) { - events.push(evt); - }, - }; - - syncSelectionClasses(grid, new Set(["2"]), () => cards); - assert.equal(cards[1].classList.has("is-selected"), true); - assert.equal(cards[0].getAttribute("aria-selected"), "false"); - - const selected = setSelectionIds(grid, ["1", 3, "3"], { activeId: "3" }, () => cards); - assert.deepEqual(selected.sort(), ["1", "3"]); - assert.equal(grid.dataset.mjrSelectedAssetId, "3"); - assert.ok(events.length >= 1); -}); - -test("safeEscapeId falls back when CSS.escape is unavailable", () => { - globalThis.CSS = undefined; - assert.equal(safeEscapeId('a"b\\c'), 'a\\"b\\\\c'); -}); - diff --git a/js/tests/grid_selection_manager.vitest.mjs b/js/tests/grid_selection_manager.vitest.mjs new file mode 100644 index 00000000..eeda05b5 --- /dev/null +++ b/js/tests/grid_selection_manager.vitest.mjs @@ -0,0 +1,74 @@ +import { describe, expect, it } from "vitest"; + +import { + getSelectedIdSet, + safeEscapeId, + setSelectionIds, + syncSelectionClasses, +} from "../features/grid/GridSelectionManager.js"; + +function makeCard(id) { + const classSet = new Set(); + const attrs = new Map(); + return { + dataset: { mjrAssetId: String(id) }, + classList: { + add(v) { + classSet.add(v); + }, + remove(v) { + classSet.delete(v); + }, + has(v) { + return classSet.has(v); + }, + }, + setAttribute(k, v) { + attrs.set(k, v); + }, + getAttribute(k) { + return attrs.get(k); + }, + }; +} + +describe("GridSelectionManager", () => { + it("supports single and multi selection dataset keys", () => { + const g1 = { dataset: { mjrSelectedAssetIds: JSON.stringify([1, "2"]) } }; + expect(Array.from(getSelectedIdSet(g1)).sort()).toEqual(["1", "2"]); + + const g2 = { dataset: { mjrSelectedAssetId: "9" } }; + expect(Array.from(getSelectedIdSet(g2))).toEqual(["9"]); + }); + + it("updates cards and emits event when selection changes", () => { + const cards = [makeCard(1), makeCard(2), makeCard(3)]; + const events = []; + globalThis.CustomEvent = class { + constructor(type, init) { + this.type = type; + this.detail = init?.detail; + } + }; + const grid = { + dataset: {}, + dispatchEvent(evt) { + events.push(evt); + }, + }; + + syncSelectionClasses(grid, new Set(["2"]), () => cards); + expect(cards[1].classList.has("is-selected")).toBe(true); + expect(cards[0].getAttribute("aria-selected")).toBe("false"); + + const selected = setSelectionIds(grid, ["1", 3, "3"], { activeId: "3" }, () => cards); + expect(selected.sort()).toEqual(["1", "3"]); + expect(grid.dataset.mjrSelectedAssetId).toBe("3"); + expect(events.length).toBeGreaterThanOrEqual(1); + }); + + it("falls back when CSS.escape is unavailable", () => { + globalThis.CSS = undefined; + expect(safeEscapeId('a"b\\c')).toBe('a\\"b\\\\c'); + }); +}); diff --git a/mjr_am_backend/adapters/db/connection_pool.py b/mjr_am_backend/adapters/db/connection_pool.py index 4f126d9d..1205922e 100644 --- a/mjr_am_backend/adapters/db/connection_pool.py +++ b/mjr_am_backend/adapters/db/connection_pool.py @@ -11,6 +11,7 @@ from typing import Any from ...shared import get_logger logger = get_logger(__name__) +_MAX_DB_CONFIG_BYTES = 1024 * 1024 def init_db(sqlite_obj: Any) -> None: @@ -35,13 +36,29 @@ def load_user_db_config(sqlite_obj: Any) -> dict[str, Any]: def resolve_user_db_config_path(sqlite_obj: Any) -> Path: + default_path = sqlite_obj.db_path.parent / "db_config.json" cfg_path_raw = os.getenv("MAJOOR_DB_CONFIG_PATH", "").strip() - return Path(cfg_path_raw).expanduser() if cfg_path_raw else (sqlite_obj.db_path.parent / "db_config.json") + if not cfg_path_raw: + return default_path + try: + candidate = Path(cfg_path_raw).expanduser().resolve(strict=False) + base_dir = sqlite_obj.db_path.parent.resolve(strict=False) + candidate.relative_to(base_dir) + return candidate + except Exception: + logger.warning("Ignoring MAJOOR_DB_CONFIG_PATH outside index directory: %s", cfg_path_raw) + return default_path def read_user_db_config_file(cfg_path: Path) -> Any: if not cfg_path.exists() or not cfg_path.is_file(): return {} + try: + if cfg_path.stat().st_size > _MAX_DB_CONFIG_BYTES: + logger.warning("Ignoring oversized db config file (%s bytes): %s", cfg_path.stat().st_size, cfg_path) + return {} + except Exception: + return {} with cfg_path.open("r", encoding="utf-8") as f: return json.load(f) diff --git a/mjr_am_backend/adapters/db/db_recovery.py b/mjr_am_backend/adapters/db/db_recovery.py index 9eb9c39d..bf76d964 100644 --- a/mjr_am_backend/adapters/db/db_recovery.py +++ b/mjr_am_backend/adapters/db/db_recovery.py @@ -2,6 +2,7 @@ Database recovery and diagnostics helpers extracted from sqlite.py. """ import os +import re import sqlite3 import time from pathlib import Path @@ -10,6 +11,21 @@ from typing import Any from ...shared import get_logger logger = get_logger(__name__) +_SAFE_IDENTIFIER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def _is_safe_identifier(value: str) -> bool: + try: + return bool(_SAFE_IDENTIFIER_RE.match(str(value or ""))) + except Exception: + return False + + +def _quoted_identifier(value: str) -> str: + if not _is_safe_identifier(value): + raise ValueError(f"Invalid identifier: {value!r}") + safe = str(value).replace('"', '""') + return f'"{safe}"' def is_locked_error(exc: Exception) -> bool: @@ -21,8 +37,7 @@ def is_locked_error(exc: Exception) -> bool: "database is locked" in msg or "database table is locked" in msg or "database schema is locked" in msg - or "busy" in msg - or "locked" in msg + or "sqlite_busy" in msg ) @@ -121,7 +136,12 @@ def populate_table_columns( known_columns: set[str], ) -> None: try: - cursor.execute(f"PRAGMA table_info({table})") + table_ident = _quoted_identifier(table) + except ValueError: + logger.warning("Refusing unsafe table identifier in schema refresh: %r", table) + return + try: + cursor.execute(f"PRAGMA table_info({table_ident})") for row in cursor.fetchall() or []: col = extract_schema_column_name(row) if not col: @@ -129,8 +149,8 @@ def populate_table_columns( known_columns.add(col) if alias: known_columns.add(f"{alias}.{col}") - except Exception: - pass + except Exception as exc: + logger.debug("Failed to inspect table columns for %s: %s", table, exc) def populate_known_columns_from_schema( diff --git a/mjr_am_backend/adapters/db/schema.py b/mjr_am_backend/adapters/db/schema.py index 804eda0d..e4a4700e 100644 --- a/mjr_am_backend/adapters/db/schema.py +++ b/mjr_am_backend/adapters/db/schema.py @@ -198,10 +198,19 @@ def _is_safe_identifier(value: str) -> bool: return bool(value and isinstance(value, str) and _SAFE_IDENT_RE.match(value)) +def _quoted_identifier(value: str) -> str: + if not _is_safe_identifier(value): + raise ValueError(f"Invalid identifier: {value!r}") + safe = value.replace('"', '""') + return f'"{safe}"' + + async def _get_table_columns(db, table_name: str) -> Result[list[str]]: - if not _is_safe_identifier(table_name): + try: + table_ident = _quoted_identifier(table_name) + except ValueError: return Result.Err("INVALID_INPUT", f"Invalid table name: {table_name}") - result = await db.aquery(f"PRAGMA table_info('{table_name}')") + result = await db.aquery(f"PRAGMA table_info({table_ident})") if not result.ok: return Result.Err("PRAGMA_FAILED", f"Unable to inspect {table_name}: {result.error}") return Result.Ok([row["name"] for row in result.data or []]) @@ -236,8 +245,12 @@ async def _ensure_column(db, table_name: str, column_name: str, definition: str) return Result.Ok(True) logger.info("Adding missing column %s.%s", table_name, column_name) + try: + table_ident = _quoted_identifier(table_name) + except ValueError: + return Result.Err("INVALID_INPUT", f"Invalid table name: {table_name}") alter_result = await db.aexecute( - f"ALTER TABLE {table_name} ADD COLUMN {definition}" + f"ALTER TABLE {table_ident} ADD COLUMN {definition}" ) return alter_result @@ -282,8 +295,11 @@ async def ensure_indexes_and_triggers(db) -> Result[bool]: async def _fts_has_column(db, table: str, col: str) -> bool: """Check if an FTS table has a specific column.""" + if not _is_safe_identifier(table) or not _is_safe_identifier(col): + return False try: - r = await db.aquery(f"PRAGMA table_info({table})") + table_ident = _quoted_identifier(table) + r = await db.aquery(f"PRAGMA table_info({table_ident})") if not r.ok or not r.data: return False return any((row.get("name") == col) for row in r.data) diff --git a/mjr_am_backend/adapters/db/sqlite_facade.py b/mjr_am_backend/adapters/db/sqlite_facade.py index ef5feff5..e3b80e08 100644 --- a/mjr_am_backend/adapters/db/sqlite_facade.py +++ b/mjr_am_backend/adapters/db/sqlite_facade.py @@ -762,9 +762,13 @@ class Sqlite: async def _apply_connection_pragmas(self, conn: aiosqlite.Connection): await conn.execute("PRAGMA journal_mode=WAL") await conn.execute("PRAGMA synchronous=NORMAL") + # Int constants only: never interpolate user-controlled values into PRAGMA SQL. + assert isinstance(SQLITE_CACHE_SIZE_KIB, int), "SQLITE_CACHE_SIZE_KIB must be int" await conn.execute(f"PRAGMA cache_size={SQLITE_CACHE_SIZE_KIB}") await conn.execute("PRAGMA temp_store=MEMORY") - await conn.execute(f"PRAGMA busy_timeout = {SQLITE_BUSY_TIMEOUT_MS}") + # Int constants only: never interpolate user-controlled values into PRAGMA SQL. + assert isinstance(SQLITE_BUSY_TIMEOUT_MS, int), "SQLITE_BUSY_TIMEOUT_MS must be int" + await conn.execute(f"PRAGMA busy_timeout={SQLITE_BUSY_TIMEOUT_MS}") await conn.execute("PRAGMA foreign_keys=ON") async def _create_connection(self) -> aiosqlite.Connection: diff --git a/mjr_am_backend/adapters/db/transaction_manager.py b/mjr_am_backend/adapters/db/transaction_manager.py index 99f97900..fa27d881 100644 --- a/mjr_am_backend/adapters/db/transaction_manager.py +++ b/mjr_am_backend/adapters/db/transaction_manager.py @@ -11,6 +11,7 @@ from typing import Any import aiosqlite +# Allow bare column names and qualified table.column identifiers. _COLUMN_NAME_PATTERN = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*(\.[a-zA-Z_][a-zA-Z0-9_]*)?$") _IN_QUERY_FORBIDDEN = re.compile( r"(--|/\*|\*/|;|\bpragma\b|\battach\b|\bdetach\b|\bvacuum\b|\balter\b|\bdrop\b|\binsert\b|\bupdate\b|\bdelete\b)", diff --git a/mjr_am_backend/adapters/tools/exiftool.py b/mjr_am_backend/adapters/tools/exiftool.py index 77cc7eb4..a8ce307d 100644 --- a/mjr_am_backend/adapters/tools/exiftool.py +++ b/mjr_am_backend/adapters/tools/exiftool.py @@ -17,6 +17,7 @@ logger = get_logger(__name__) _TAG_SAFE_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_:-]*$") _WINDOWS_CMDLINE_TOO_LONG = 206 +_MAX_WRITE_VALUE_CHARS = 8192 def _decode_bytes_best_effort(blob: bytes | None) -> tuple[str, bool]: @@ -159,6 +160,31 @@ def _build_match_map(valid_paths: list[str]) -> tuple[dict[str, list[str]], list return key_to_paths, cmd_paths +def _sanitize_exiftool_value(value: Any) -> str: + try: + text = str(value) + except Exception: + text = "" + if not text: + return "" + text = text.replace("\x00", "").replace("\r", " ").replace("\n", " ").strip() + if len(text) > _MAX_WRITE_VALUE_CHARS: + text = text[:_MAX_WRITE_VALUE_CHARS] + return text + + +def _sanitize_windows_stdin_path(path: str) -> str: + try: + text = str(path or "") + except Exception: + text = "" + if not text: + return "" + if "\x00" in text or "\n" in text or "\r" in text: + return "" + return text + + class ExifTool: """ ExifTool wrapper for metadata operations. @@ -802,7 +828,7 @@ class ExifTool: def _validate_write_preconditions(self, path: str) -> Result[bool] | None: if not self._available: return Result.Err(ErrorCode.TOOL_MISSING, "ExifTool not found in PATH") - if not path or "\x00" in str(path): + if not path or any(ch in str(path) for ch in ("\x00", "\n", "\r")): return Result.Err(ErrorCode.INVALID_INPUT, "Invalid file path") try: file_path = Path(str(path)) @@ -841,18 +867,21 @@ class ExifTool: for item in value: if item is None: continue - text = str(item).strip() + text = _sanitize_exiftool_value(item) if text: cmd.append(f"-{key}+={text}") continue - cmd.append(f"-{key}={value}") + cmd.append(f"-{key}={_sanitize_exiftool_value(value)}") @staticmethod def _append_write_target_args(cmd: list[str], path: str) -> tuple[list[str], str | None]: stdin_input: str | None = None if os.name == "nt": cmd.extend(["-charset", "filename=utf8", "-@", "-"]) - stdin_input = f"{path}\n" + safe_path = _sanitize_windows_stdin_path(path) + if not safe_path: + return cmd, None + stdin_input = f"{safe_path}\n" cmd.append("-overwrite_original") else: cmd.extend(["-overwrite_original", path]) diff --git a/mjr_am_backend/adapters/tools/ffprobe.py b/mjr_am_backend/adapters/tools/ffprobe.py index f20ce84e..af5effcc 100644 --- a/mjr_am_backend/adapters/tools/ffprobe.py +++ b/mjr_am_backend/adapters/tools/ffprobe.py @@ -228,7 +228,7 @@ class FFProbe: normalized = "" if not normalized: return Result.Err(ErrorCode.INVALID_INPUT, "Invalid file path") - if "\x00" in normalized: + if "\x00" in normalized or "\n" in normalized or "\r" in normalized: return Result.Err(ErrorCode.INVALID_INPUT, "Invalid file path") # Prevent ffprobe option injection via user-controlled leading dash. if normalized.startswith("-"): diff --git a/mjr_am_backend/config.py b/mjr_am_backend/config.py index a5cce79d..ac71c13b 100644 --- a/mjr_am_backend/config.py +++ b/mjr_am_backend/config.py @@ -4,11 +4,14 @@ Configuration for Majoor Assets Manager. import logging import os import sys +import threading from pathlib import Path from .utils import env_bool logger = logging.getLogger(__name__) +_INIT_LOCK = threading.Lock() +_DIRS_INITIALIZED = False def _env_raw(*names: str, default: str | None = None) -> str | None: @@ -150,7 +153,12 @@ def _resolve_output_root_fallback() -> Path: return fallback_dir.resolve() except (OSError, RuntimeError) as exc: logger.error("All fallback strategies failed: %s", exc) - return Path.cwd() / "output" + fallback = Path.cwd() / "output" + try: + fallback.mkdir(exist_ok=True) + except Exception as mkdir_exc: + logger.warning("Failed to create fallback output directory %s: %s", fallback, mkdir_exc) + return fallback def _resolve_output_root_from_argv() -> Path | None: @@ -211,9 +219,17 @@ INDEX_DB = str(INDEX_DB_PATH) COLLECTIONS_DIR_PATH = INDEX_DIR_PATH / "collections" COLLECTIONS_DIR = str(COLLECTIONS_DIR_PATH) -# Create index directory if it doesn't exist -os.makedirs(INDEX_DIR, exist_ok=True) -os.makedirs(COLLECTIONS_DIR, exist_ok=True) + +def initialize_directories() -> None: + global _DIRS_INITIALIZED + if _DIRS_INITIALIZED: + return + with _INIT_LOCK: + if _DIRS_INITIALIZED: + return + os.makedirs(INDEX_DIR, exist_ok=True) + os.makedirs(COLLECTIONS_DIR, exist_ok=True) + _DIRS_INITIALIZED = True # External tool overrides (portable vs. system-wide) EXIFTOOL_BIN = _env_raw("MJR_AM_EXIFTOOL_PATH", "MAJOOR_EXIFTOOL_PATH", "MAJOOR_EXIFTOOL_BIN", default="exiftool") @@ -233,7 +249,6 @@ FFPROBE_MIN_VERSION = str(_env_raw("MJR_AM_FFPROBE_MIN_VERSION", "MAJOOR_FFPROBE # Media probe backend setting # Options: "auto" (recommended), "exiftool", "ffprobe", "both" -MEDIA_PROBE_BACKEND = os.getenv("MAJOOR_MEDIA_PROBE_BACKEND", "auto") MEDIA_PROBE_BACKEND = str(_env_raw("MJR_AM_MEDIA_PROBE_BACKEND", "MAJOOR_MEDIA_PROBE_BACKEND", default="auto") or "auto") # Tool timeouts. diff --git a/mjr_am_backend/custom_roots.py b/mjr_am_backend/custom_roots.py index 67ad60f7..0319685d 100644 --- a/mjr_am_backend/custom_roots.py +++ b/mjr_am_backend/custom_roots.py @@ -199,7 +199,8 @@ def add_custom_root(path: str, label: str | None = None) -> Result[dict[str, Any validation = _validate_new_root_path(normalized) if validation is not None: return validation - assert normalized is not None + if normalized is None: + return Result.Err("INVALID_INPUT", "Invalid path") payload = _new_root_payload(normalized, label=label) resolved = payload["resolved"] diff --git a/mjr_am_backend/deps.py b/mjr_am_backend/deps.py index 4bfc1682..0c3f658f 100644 --- a/mjr_am_backend/deps.py +++ b/mjr_am_backend/deps.py @@ -18,6 +18,7 @@ from .config import ( FFPROBE_TIMEOUT, INDEX_DB, WATCHER_ENABLED, + initialize_directories, ) from .features.duplicates import DuplicatesService from .features.health import HealthService @@ -94,7 +95,8 @@ def _build_services_dict( async def _load_watcher_scope_or_default(db: Sqlite) -> dict: try: return await load_watcher_scope(db) - except Exception: + except Exception as exc: + logger.debug("Falling back to default watcher scope: %s", exc) return {"scope": "output", "custom_root_id": ""} @@ -127,6 +129,11 @@ async def build_services(db_path: str | None = None) -> Result[dict]: Result[dict] of service instances """ logger.info("Building services...") + try: + initialize_directories() + except Exception as exc: + logger.error("Failed to initialize directories: %s", exc) + return Result.Err("DB_ERROR", f"Failed to initialize directories: {exc}") db_path = _resolve_db_path(db_path) db_res = _init_db_or_error(db_path) @@ -210,7 +217,8 @@ async def _create_watcher(index_service: IndexService) -> OutputWatcher: for fp in filepaths: try: await index_service.remove_file(str(fp)) - except Exception: + except Exception as exc: + logger.debug("Watcher remove callback failed for %s: %s", fp, exc) continue async def move_callback(moves: list, _base_dir: str, _source: str | None = None, _root_id: str | None = None): @@ -219,7 +227,8 @@ async def _create_watcher(index_service: IndexService) -> OutputWatcher: for move in moves: try: old_fp, new_fp = move - except Exception: + except Exception as exc: + logger.debug("Watcher move payload invalid (%r): %s", move, exc) continue try: res = await index_service.rename_file(str(old_fp), str(new_fp)) @@ -232,7 +241,8 @@ async def _create_watcher(index_service: IndexService) -> OutputWatcher: source=_source or "watcher", root_id=_root_id, ) - except Exception: + except Exception as exc: + logger.debug("Watcher move callback failed (%s -> %s): %s", old_fp, new_fp, exc) continue watcher = OutputWatcher(index_callback, remove_callback=remove_callback, move_callback=move_callback) @@ -240,7 +250,8 @@ async def _create_watcher(index_service: IndexService) -> OutputWatcher: # Collect directories to watch try: scope_cfg = await load_watcher_scope(index_service.db) - except Exception: + except Exception as exc: + logger.debug("Failed to load watcher scope; using defaults: %s", exc) scope_cfg = {"scope": "output", "custom_root_id": ""} scope = str((scope_cfg or {}).get("scope") or "output") @@ -248,7 +259,7 @@ async def _create_watcher(index_service: IndexService) -> OutputWatcher: watch_paths = build_watch_paths(scope, custom_root_id) if watch_paths: - loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop() await watcher.start(watch_paths, loop) return watcher diff --git a/mjr_am_backend/observability.py b/mjr_am_backend/observability.py index 53254536..9ec02286 100644 --- a/mjr_am_backend/observability.py +++ b/mjr_am_backend/observability.py @@ -61,6 +61,8 @@ def _is_client_disconnect(exc: BaseException) -> bool: logger = get_logger(__name__) _APPKEY_OBS_INSTALLED = web.AppKey("mjr_observability_installed", bool) +_OBS_INSTALL_LOCK = threading.Lock() +_ASYNCIO_HANDLER_LOCK = threading.Lock() # Time constants (ms) / tunables MS_PER_S = 1000.0 @@ -75,6 +77,19 @@ _DEFAULT_RATELIMIT_CLEAN_MIN_CUTOFF_MS = 120_000.0 _LOG_RATELIMIT_LOCK = threading.Lock() _LOG_RATELIMIT_STATE: dict[str, float] = {} _LOG_RATELIMIT_CLEAN_AT = 0.0 +_SENSITIVE_QUERY_KEYS = frozenset( + { + "token", + "api_token", + "apikey", + "api_key", + "authorization", + "auth", + "password", + "secret", + "key", + } +) # Runtime-configured values are read from env at call time (tests rely on monkeypatching env). @@ -345,17 +360,18 @@ def ensure_observability(app: web.Application) -> None: """ Install middleware once. """ - try: - if app.get(_APPKEY_OBS_INSTALLED): + with _OBS_INSTALL_LOCK: + try: + if app.get(_APPKEY_OBS_INSTALLED): + return + app[_APPKEY_OBS_INSTALLED] = True + except Exception: return - app[_APPKEY_OBS_INSTALLED] = True - except Exception: - return - try: - app.middlewares.append(request_context_middleware) - except Exception as exc: - logger.debug("Failed to install observability middleware: %s", exc) + try: + app.middlewares.append(request_context_middleware) + except Exception as exc: + logger.debug("Failed to install observability middleware: %s", exc) # Install asyncio exception handler to silence benign client disconnects. # These errors occur at the transport level (after middleware) when clients @@ -376,41 +392,64 @@ def _install_asyncio_exception_handler() -> None: connection before the server finished sending data. """ global _ASYNCIO_HANDLER_INSTALLED - if _ASYNCIO_HANDLER_INSTALLED: - return - - try: - loop = asyncio.get_running_loop() - except RuntimeError: - # No running loop yet (e.g., during module import). - # This is fine; the handler will be installed when ensure_observability - # is called at runtime with an active loop. - return - - original_handler = loop.get_exception_handler() - - def _quiet_exception_handler(loop: asyncio.AbstractEventLoop, context: dict[str, Any]) -> None: - """ - Custom exception handler that silences client disconnect errors. - """ - exc = context.get("exception") - if exc is not None and _is_client_disconnect(exc): - # Silently ignore client disconnect errors. - # These are benign and expected in web servers. + with _ASYNCIO_HANDLER_LOCK: + if _ASYNCIO_HANDLER_INSTALLED: return - # For all other exceptions, use the original handler or default. - if original_handler is not None: - original_handler(loop, context) - else: - loop.default_exception_handler(context) + try: + loop = asyncio.get_running_loop() + except RuntimeError: + # No running loop yet (e.g., during module import). + # This is fine; the handler will be installed when ensure_observability + # is called at runtime with an active loop. + return + original_handler = loop.get_exception_handler() + + def _quiet_exception_handler(loop: asyncio.AbstractEventLoop, context: dict[str, Any]) -> None: + """ + Custom exception handler that silences client disconnect errors. + """ + exc = context.get("exception") + if exc is not None and _is_client_disconnect(exc): + # Silently ignore client disconnect errors. + # These are benign and expected in web servers. + return + + # For all other exceptions, use the original handler or default. + if original_handler is not None: + original_handler(loop, context) + else: + loop.default_exception_handler(context) + + try: + loop.set_exception_handler(_quiet_exception_handler) + _ASYNCIO_HANDLER_INSTALLED = True + logger.debug("Installed asyncio exception handler for client disconnect errors") + except Exception as exc: + logger.debug("Failed to install asyncio exception handler: %s", exc) + + +def _sanitize_query_params(request: web.Request) -> dict[str, Any]: + out: dict[str, Any] = {} try: - loop.set_exception_handler(_quiet_exception_handler) - _ASYNCIO_HANDLER_INSTALLED = True - logger.debug("Installed asyncio exception handler for client disconnect errors") - except Exception as exc: - logger.debug("Failed to install asyncio exception handler: %s", exc) + iterator = request.query.items() + except Exception: + return out + for key, value in iterator: + k = str(key or "") + lower = k.lower() + if lower in _SENSITIVE_QUERY_KEYS or lower.endswith("_token") or lower.endswith("_key"): + out[k] = "[redacted]" + continue + try: + text = str(value or "") + except Exception: + text = "" + if len(text) > 256: + text = text[:256] + "..." + out[k] = text + return out def build_request_log_fields(request: web.Request, response_status: int | None = None) -> dict[str, Any]: @@ -420,7 +459,7 @@ def build_request_log_fields(request: web.Request, response_status: int | None = "request_id": request.get("mjr_request_id"), "method": request.method, "path": request.path, - "query": dict(request.query), + "query": _sanitize_query_params(request), "status": response_status, "duration_ms": request.get("mjr_duration_ms"), "remote": getattr(request, "remote", None), diff --git a/mjr_am_backend/routes/assets/filename_validator.py b/mjr_am_backend/routes/assets/filename_validator.py index c5dc3eb5..6fe36395 100644 --- a/mjr_am_backend/routes/assets/filename_validator.py +++ b/mjr_am_backend/routes/assets/filename_validator.py @@ -2,8 +2,8 @@ _WINDOWS_RESERVED = { 'CON', 'PRN', 'AUX', 'NUL', - 'COM1', 'COM2', 'COM3', 'COM4', 'COM5', 'COM6', 'COM7', 'COM8', 'COM9', - 'LPT1', 'LPT2', 'LPT3', 'LPT4', 'LPT5', 'LPT6', 'LPT7', 'LPT8', 'LPT9' + 'COM0', 'COM1', 'COM2', 'COM3', 'COM4', 'COM5', 'COM6', 'COM7', 'COM8', 'COM9', + 'LPT0', 'LPT1', 'LPT2', 'LPT3', 'LPT4', 'LPT5', 'LPT6', 'LPT7', 'LPT8', 'LPT9' } diff --git a/mjr_am_backend/routes/core/request_json.py b/mjr_am_backend/routes/core/request_json.py index 35ff9c0c..14237192 100644 --- a/mjr_am_backend/routes/core/request_json.py +++ b/mjr_am_backend/routes/core/request_json.py @@ -59,14 +59,14 @@ async def _read_json(request: web.Request, *, max_bytes: int | None = None) -> R def _content_type_error(request: web.Request) -> Result[dict] | None: """ - Enforce JSON content type when explicitly provided. + Enforce JSON content type. """ try: content_type = str(request.headers.get("Content-Type") or "").strip().lower() except Exception: content_type = "" if not content_type: - return None + return Result.Err(ErrorCode.INVALID_INPUT, "Missing Content-Type header (application/json required)") media_type = content_type.split(";", 1)[0].strip() if media_type == "application/json" or media_type.endswith("+json"): return None diff --git a/mjr_am_backend/routes/core/security.py b/mjr_am_backend/routes/core/security.py index 9eea1950..4641f04d 100644 --- a/mjr_am_backend/routes/core/security.py +++ b/mjr_am_backend/routes/core/security.py @@ -12,34 +12,30 @@ import time from collections import OrderedDict from collections.abc import Mapping from functools import lru_cache +from http.cookies import SimpleCookie from typing import Any from urllib.parse import urlparse from aiohttp import web from multidict import CIMultiDict -from mjr_am_backend.shared import Result +from mjr_am_backend.shared import Result, get_logger + +logger = get_logger(__name__) # Per-client rate limiting state: {client_id: {endpoint: [timestamps]}} # Use LRU eviction to prevent unbounded memory growth from spoofed client IPs. _DEFAULT_MAX_RATE_LIMIT_CLIENTS = 1_000 _DEFAULT_MAX_RATE_LIMIT_ENDPOINTS_PER_CLIENT = 32 -_DEFAULT_RATE_LIMIT_CLEANUP_INTERVAL = 100 _DEFAULT_RATE_LIMIT_MIN_WINDOW_SECONDS = 60 _DEFAULT_CLIENT_ID_HASH_HEX_CHARS = 16 _DEFAULT_TRUSTED_PROXIES = "127.0.0.1,::1" +_DEFAULT_RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS = 60 try: _MAX_RATE_LIMIT_CLIENTS = int(os.environ.get("MAJOOR_RATE_LIMIT_MAX_CLIENTS", str(_DEFAULT_MAX_RATE_LIMIT_CLIENTS))) except Exception: _MAX_RATE_LIMIT_CLIENTS = _DEFAULT_MAX_RATE_LIMIT_CLIENTS -try: - _rate_limit_cleanup_interval = int( - os.environ.get("MAJOOR_RATE_LIMIT_CLEANUP_INTERVAL", str(_DEFAULT_RATE_LIMIT_CLEANUP_INTERVAL)) - ) -except Exception: - _rate_limit_cleanup_interval = _DEFAULT_RATE_LIMIT_CLEANUP_INTERVAL - try: _RATE_LIMIT_MIN_WINDOW_SECONDS = int( os.environ.get("MAJOOR_RATE_LIMIT_MIN_WINDOW_SECONDS", str(_DEFAULT_RATE_LIMIT_MIN_WINDOW_SECONDS)) @@ -68,7 +64,19 @@ _rate_limit_state: OrderedDict[str, dict[str, list[float]]] = OrderedDict() # threading.Lock is intentional here: rate-limit updates are synchronous and never await. # Do not add `await` inside a `with _rate_limit_lock:` critical section. _rate_limit_lock = threading.Lock() -_rate_limit_cleanup_counter = 0 +_rate_limit_cleanup_thread: threading.Thread | None = None +_rate_limit_cleanup_thread_lock = threading.Lock() +_rate_limit_cleanup_stop = threading.Event() +try: + _RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS = int( + os.environ.get( + "MAJOOR_RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS", + str(_DEFAULT_RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS), + ) + ) +except Exception: + _RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS = _DEFAULT_RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS +_RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS = max(10, _RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS) def _env_truthy(name: str, default: bool = False) -> bool: @@ -81,6 +89,7 @@ def _env_truthy(name: str, default: bool = False) -> bool: return str(raw).strip().lower() in ("1", "true", "yes", "on") +@lru_cache(maxsize=1) def _safe_mode_enabled() -> bool: """ Return True when Majoor Safe Mode is enabled. @@ -277,7 +286,23 @@ def _extract_write_token_from_headers(headers: Mapping[str, str]) -> str: token = str(headers.get("X-MJR-Token") or "").strip() except Exception: token = "" - return token + if token: + return token + + try: + cookie_header = str(headers.get("Cookie") or "").strip() + except Exception: + cookie_header = "" + if cookie_header: + try: + parsed = SimpleCookie() + parsed.load(cookie_header) + raw = parsed.get("mjr_write_token") + if raw is not None: + return str(raw.value or "").strip() + except Exception: + pass + return "" def _resolve_client_ip(peer_ip: str, headers: Mapping[str, str]) -> str: @@ -338,14 +363,6 @@ def _client_ip_from_forwarded_chain(forwarded_chain: list[str], *, peer: str) -> return "" -def _forwarded_for_ip(headers: Mapping[str, str]) -> str: - forwarded_for = _header_value(headers, "X-Forwarded-For") - if not forwarded_for: - return "" - candidate = forwarded_for.split(",")[0].strip() - return candidate if _is_valid_ip(candidate) else "" - - def _real_ip_from_header(headers: Mapping[str, str]) -> str: real_ip = _header_value(headers, "X-Real-IP") if not real_ip: @@ -380,11 +397,12 @@ def _check_write_access(*, peer_ip: str, headers: Mapping[str, str], request_sch Authorization guard for destructive/write endpoints. Default policy: - - If a token is configured, it must be provided for *all* write operations. - - If no token is configured, allow loopback clients only by default. + - Loopback clients are trusted by default (same machine = same user). + - If a token is configured AND the client is remote: token is required. + - If no token is configured: allow loopback, deny remote. Overrides: - - MAJOOR_REQUIRE_AUTH=1 forces token auth even for loopback (requires MAJOOR_API_TOKEN). + - MAJOOR_REQUIRE_AUTH=1 forces token auth even for loopback. - MAJOOR_ALLOW_REMOTE_WRITE=1 allows remote writes when no token is set. Returns a Result that never raises (route handlers should return 200 with this Result on error). @@ -403,7 +421,7 @@ def _check_write_access(*, peer_ip: str, headers: Mapping[str, str], request_sch provided = _extract_write_token_from_headers(headers) if configured_hash: - # Prevent timing attacks + # If the client provides a valid token, always accept it (any transport policy applies). if provided and hmac.compare_digest(_hash_token(provided), configured_hash): if not _request_transport_is_secure(peer_ip=peer_ip, headers=headers, request_scheme=request_scheme): if not _env_truthy("MAJOOR_ALLOW_INSECURE_TOKEN_TRANSPORT", default=False): @@ -414,6 +432,12 @@ def _check_write_access(*, peer_ip: str, headers: Mapping[str, str], request_sch client_ip=client_ip, ) return Result.Ok(True, auth="token", client_ip=client_ip) + # Token missing or invalid. + # Loopback clients (same machine) are trusted by default unless MAJOOR_REQUIRE_AUTH=1 + # is explicitly set. This prevents auto-generated or rotated token hashes that were + # persisted in the database from locking out local users who never configured a token. + if not require_auth and _is_loopback_ip(client_ip): + return Result.Ok(True, auth="loopback", client_ip=client_ip) return Result.Err( "AUTH_REQUIRED", "Write operation blocked: missing or invalid API token. Set MAJOOR_API_TOKEN and send it via X-MJR-Token or Authorization: Bearer .", @@ -656,6 +680,15 @@ def _parse_trusted_proxies() -> list[ipaddress._BaseNetwork]: _TRUSTED_PROXY_NETS = _parse_trusted_proxies() +def _refresh_trusted_proxy_cache() -> None: + global _TRUSTED_PROXY_NETS + _TRUSTED_PROXY_NETS = _parse_trusted_proxies() + try: + _is_trusted_proxy.cache_clear() + except Exception: + pass + + @lru_cache(maxsize=2048) def _is_trusted_proxy(ip: str) -> bool: try: @@ -684,27 +717,11 @@ def _extract_peer_ip(request: web.Request) -> str: def _get_client_identifier(request: web.Request) -> str: peer_ip = _extract_peer_ip(request) - - # Only trust X-Forwarded-For / X-Real-IP when we are behind a trusted proxy. - forwarded_for = request.headers.get("X-Forwarded-For") if _is_trusted_proxy(peer_ip) else None - if forwarded_for: - cand = forwarded_for.split(",")[0].strip() - try: - ipaddress.ip_address(cand) - client_ip = cand - except Exception: - client_ip = peer_ip - else: - real_ip = request.headers.get("X-Real-IP") if _is_trusted_proxy(peer_ip) else None - if real_ip: - cand = real_ip.strip() - try: - ipaddress.ip_address(cand) - client_ip = cand - except Exception: - client_ip = peer_ip - else: - client_ip = peer_ip + try: + headers = request.headers + except Exception: + headers = CIMultiDict() + client_ip = _resolve_client_ip(peer_ip, headers) return hashlib.sha256(client_ip.encode("utf-8", errors="ignore")).hexdigest()[:_CLIENT_ID_HASH_HEX_CHARS] def _evict_oldest_clients_if_needed() -> None: @@ -758,6 +775,30 @@ def _cleanup_rate_limit_state_locked(now: float, window_seconds: int) -> None: _rate_limit_state.pop(client_id, None) +def _rate_limit_cleanup_loop() -> None: + while not _rate_limit_cleanup_stop.wait(timeout=float(_RATE_LIMIT_BACKGROUND_CLEANUP_SECONDS)): + try: + now = time.time() + with _rate_limit_lock: + _cleanup_rate_limit_state_locked(now, max(_RATE_LIMIT_MIN_WINDOW_SECONDS, 60)) + except Exception as exc: + logger.debug("Rate limit background cleanup failed: %s", exc) + + +def _ensure_rate_limit_cleanup_thread() -> None: + global _rate_limit_cleanup_thread + with _rate_limit_cleanup_thread_lock: + if _rate_limit_cleanup_thread and _rate_limit_cleanup_thread.is_alive(): + return + _rate_limit_cleanup_stop.clear() + _rate_limit_cleanup_thread = threading.Thread( + target=_rate_limit_cleanup_loop, + name="mjr-rate-limit-cleanup", + daemon=True, + ) + _rate_limit_cleanup_thread.start() + + def _check_rate_limit( request: web.Request, endpoint: str, @@ -770,22 +811,18 @@ def _check_rate_limit( Returns: (allowed, retry_after_seconds) """ - global _rate_limit_cleanup_counter + _ensure_rate_limit_cleanup_thread() try: client_id = _get_client_identifier(request) now = time.time() - except Exception: + except Exception as exc: # If we can't identify the client, fail open to avoid breaking UI. + logger.warning("Rate limit identity resolution failed; failing open: %s", exc) return True, None try: with _rate_limit_lock: - _rate_limit_cleanup_counter += 1 - if _rate_limit_cleanup_counter >= _rate_limit_cleanup_interval: - _cleanup_rate_limit_state_locked(now, max(window_seconds, _RATE_LIMIT_MIN_WINDOW_SECONDS)) - _rate_limit_cleanup_counter = 0 - client_state = _get_or_create_client_state(client_id) _evict_oldest_endpoint_if_needed(client_state, endpoint) previous = client_state.get(endpoint, []) @@ -798,7 +835,8 @@ def _check_rate_limit( recent.append(now) client_state[endpoint] = recent return True, None - except Exception: + except Exception as exc: + logger.error("Rate limit check raised unexpectedly; failing open: %s", exc, exc_info=True) return True, None @@ -894,3 +932,15 @@ def _reset_security_state_for_tests() -> None: _rate_limit_state.clear() except Exception: pass + try: + _rate_limit_cleanup_stop.set() + except Exception: + pass + try: + _is_trusted_proxy.cache_clear() + except Exception: + pass + try: + _safe_mode_enabled.cache_clear() + except Exception: + pass diff --git a/mjr_am_backend/routes/core/services.py b/mjr_am_backend/routes/core/services.py index a4dc1314..bdc26cf3 100644 --- a/mjr_am_backend/routes/core/services.py +++ b/mjr_am_backend/routes/core/services.py @@ -2,6 +2,7 @@ Service management and initialization. """ import asyncio +import threading from typing import Any from mjr_am_backend.deps import build_services @@ -11,7 +12,18 @@ logger = get_logger(__name__) _services = None _services_error = None -_services_lock = asyncio.Lock() +_services_lock: asyncio.Lock | None = None +_services_lock_guard = threading.Lock() + + +def _get_services_lock() -> asyncio.Lock: + global _services_lock + if _services_lock is not None: + return _services_lock + with _services_lock_guard: + if _services_lock is None: + _services_lock = asyncio.Lock() + return _services_lock async def _dispose_services(): @@ -46,7 +58,7 @@ async def _dispose_services(): async def _build_services(force: bool = False): global _services, _services_error - async with _services_lock: + async with _get_services_lock(): if _services and not force: return _services diff --git a/mjr_am_backend/routes/handlers/assets_impl.py b/mjr_am_backend/routes/handlers/assets_impl.py index 473f6ebb..393cb5df 100644 --- a/mjr_am_backend/routes/handlers/assets_impl.py +++ b/mjr_am_backend/routes/handlers/assets_impl.py @@ -554,6 +554,9 @@ def register_asset_routes(routes: web.RouteTableDef) -> None: csrf = _csrf_error(request) if csrf: return _json_response(Result.Err("CSRF", csrf)) + auth = _require_write_access(request) + if not auth.ok: + return _json_response(auth) svc, error_result = await _require_services() if error_result: @@ -562,9 +565,6 @@ def register_asset_routes(routes: web.RouteTableDef) -> None: op = _require_operation_enabled("open_in_folder", prefs=prefs) if not op.ok: return _json_response(op) - auth = _require_write_access(request) - if not auth.ok: - return _json_response(auth) allowed, retry_after = _check_rate_limit(request, "open_in_folder", max_requests=1, window_seconds=2) if not allowed: @@ -632,6 +632,7 @@ def register_asset_routes(routes: web.RouteTableDef) -> None: selected = True elif os.name == "nt": commands.append(["explorer.exe", f"/select,{str(resolved)}"]) + fallback_command = ["explorer.exe", str(resolved.parent)] selected = True else: commands.append(["xdg-open", str(resolved.parent)]) @@ -660,17 +661,6 @@ def register_asset_routes(routes: web.RouteTableDef) -> None: except Exception as exc: last_exception = exc - if os.name == "nt": - try: - os.startfile(str(resolved.parent)) - return _json_response( - Result.Ok( - {"opened": True, "selected": False, "fallback": "startfile"} - ) - ) - except Exception as exc: - last_exception = exc - return _json_response( Result.Err( "DEGRADED", diff --git a/mjr_am_backend/routes/handlers/batch_zip.py b/mjr_am_backend/routes/handlers/batch_zip.py index 81792586..ef025b39 100644 --- a/mjr_am_backend/routes/handlers/batch_zip.py +++ b/mjr_am_backend/routes/handlers/batch_zip.py @@ -8,8 +8,11 @@ from __future__ import annotations import asyncio import os +import re +import stat import threading import time +import uuid import zipfile from pathlib import Path from typing import Any @@ -37,6 +40,9 @@ _BATCH_LOCK = threading.Lock() _BATCH_CACHE: dict[str, dict[str, Any]] = {} _TOKEN_LOCKS: dict[str, threading.Lock] = {} _TOKEN_LOCKS_LOCK = threading.Lock() +_CLEANUP_THREAD: threading.Thread | None = None +_CLEANUP_THREAD_LOCK = threading.Lock() +_CLEANUP_STOP = threading.Event() def _get_token_lock(token: str) -> threading.Lock: @@ -94,6 +100,59 @@ _BUILD_TIMEOUT_S = _env_float("MAJOOR_BATCH_ZIP_BUILD_TIMEOUT_S", _DEFAULT_BUILD _ZIP_COPY_CHUNK_BYTES = _env_int("MAJOOR_BATCH_ZIP_CHUNK_BYTES", _DEFAULT_ZIP_COPY_CHUNK_BYTES, minimum=16 * 1024) +def _cleanup_interval_seconds() -> float: + return max(30.0, min(float(_BATCH_TTL_SECONDS), 300.0)) + + +def _run_cleanup_loop() -> None: + interval = _cleanup_interval_seconds() + consecutive_errors = 0 + while True: + wait_seconds = min(interval * (2 ** min(consecutive_errors, 6)), 3600.0) + if _CLEANUP_STOP.wait(timeout=wait_seconds): + break + try: + _cleanup_batch_zips() + consecutive_errors = 0 + except Exception as exc: + consecutive_errors += 1 + logger.warning( + "Batch ZIP cleanup failed (attempt %s): %s", + consecutive_errors, + exc, + exc_info=True, + ) + + +def _ensure_cleanup_thread() -> None: + global _CLEANUP_THREAD + with _CLEANUP_THREAD_LOCK: + if _CLEANUP_THREAD and _CLEANUP_THREAD.is_alive(): + return + _CLEANUP_STOP.clear() + _CLEANUP_THREAD = threading.Thread( + target=_run_cleanup_loop, + name="mjr-batch-zip-cleaner", + daemon=True, + ) + _CLEANUP_THREAD.start() + + +def stop_cleanup_thread(timeout: float = 2.0) -> None: + global _CLEANUP_THREAD + with _CLEANUP_THREAD_LOCK: + _CLEANUP_STOP.set() + thread = _CLEANUP_THREAD + if thread and thread.is_alive(): + try: + thread.join(timeout=max(0.1, float(timeout))) + except Exception: + pass + with _CLEANUP_THREAD_LOCK: + if _CLEANUP_THREAD is thread and (thread is None or not thread.is_alive()): + _CLEANUP_THREAD = None + + def _sanitize_token(token: Any) -> str: raw = str(token or "").strip() if not raw: @@ -151,7 +210,10 @@ def _batch_file_response(entry: dict[str, Any], token: str) -> web.StreamRespons if not isinstance(path, Path) or not path.exists(): return _json_response(Result.Err("NOT_FOUND", "File missing"), status=404) name = entry.get("filename") or f"{token}.zip" - safe_name = str(name).replace('"', "").replace(";", "").replace("\r", "").replace("\n", "")[:_ZIP_NAME_MAX_LEN] + safe_name = re.sub(r'[^\x20-\x7e]', "_", str(name)).strip() + safe_name = safe_name.replace('"', "_").replace(";", "_")[:_ZIP_NAME_MAX_LEN] + if not safe_name: + safe_name = f"{token}.zip" headers = {"Content-Disposition": f'attachment; filename="{safe_name}"'} try: return web.FileResponse(path, headers=headers) @@ -195,17 +257,17 @@ def _delete_batch_zip_path(entry: Any, token: str, success_log: str) -> None: logger.warning("Failed to cleanup batch zip %s: %s", path.name if path else token, exc) -def _resolve_item_path(item: dict[str, Any]) -> Path | None: +def _resolve_item_path(item: dict[str, Any]) -> tuple[Path | None, Path | None]: filename_rel, subfolder_rel, typ = _normalized_batch_zip_item_parts(item) if filename_rel is None or subfolder_rel is None: - return None + return None, None base_dir = _resolve_base_dir(item, typ) if base_dir is None: - return None + return None, None candidate = _resolve_candidate_path(base_dir, subfolder_rel, filename_rel) if not _is_valid_candidate_path(candidate, base_dir): - return None - return candidate + return None, None + return candidate, base_dir def _normalized_batch_zip_item_parts(item: dict[str, Any]) -> tuple[Path | None, Path | None, str]: @@ -226,7 +288,7 @@ def _is_valid_candidate_path(candidate: Path | None, base_dir: Path) -> bool: return False if not _is_within_root(candidate, base_dir): return False - return candidate.exists() and candidate.is_file() + return True def _resolve_base_dir(item: dict[str, Any], typ: str) -> Path | None: @@ -269,6 +331,8 @@ def _resolve_candidate_path(base_dir: Path, subfolder_rel: Path, filename_rel: P def register_batch_zip_routes(routes: web.RouteTableDef) -> None: """Register batch-zip creation and download routes.""" + _ensure_cleanup_thread() + @routes.post("/mjr/am/batch-zip") async def create_batch_zip(request: web.Request) -> web.Response: loop = asyncio.get_running_loop() @@ -350,7 +414,7 @@ def register_batch_zip_routes(routes: web.RouteTableDef) -> None: for raw in items: if not isinstance(raw, dict): continue - target = _resolve_item_path(raw) + target, base_dir = _resolve_item_path(raw) if not target: continue @@ -374,25 +438,39 @@ def register_batch_zip_routes(routes: web.RouteTableDef) -> None: candidate = arc_base[:_ZIP_NAME_MAX_LEN] if candidate in used_names: n = 2 - while True: + max_attempts = max(32, len(items) + 200) + attempts = 0 + while attempts < max_attempts: attempt = f"{stem} ({n}){suffix}" if suffix else f"{stem} ({n})" attempt = attempt[:_ZIP_NAME_MAX_LEN] if attempt not in used_names: candidate = attempt break n += 1 + attempts += 1 + if candidate in used_names: + suffix_part = suffix if suffix else "" + candidate = f"{uuid.uuid4().hex[:12]}{suffix_part}"[:_ZIP_NAME_MAX_LEN] + while candidate in used_names: + candidate = f"{uuid.uuid4().hex[:12]}{suffix_part}"[:_ZIP_NAME_MAX_LEN] used_names.add(candidate) arc = candidate try: - ok = _zip_add_file_open_handle(zf, target, arc) + ok = _zip_add_file_open_handle(zf, target, arc, base_dir=base_dir) if ok: count += 1 except Exception: continue return count - def _zip_add_file_open_handle(zf: zipfile.ZipFile, path: Path, arcname: str) -> bool: + def _zip_add_file_open_handle( + zf: zipfile.ZipFile, + path: Path, + arcname: str, + *, + base_dir: Path | None = None, + ) -> bool: """ Avoid TOCTOU by opening the file once and streaming bytes into the zip entry. @@ -412,6 +490,25 @@ def register_batch_zip_routes(routes: web.RouteTableDef) -> None: st = os.fstat(f.fileno()) except Exception: st = path.stat() + try: + if not stat.S_ISREG(int(getattr(st, "st_mode", 0) or 0)): + return False + except Exception: + return False + try: + if base_dir is not None: + current = path.resolve(strict=True) + if not _is_within_root(current, base_dir): + return False + path_st = path.stat() + if ( + hasattr(st, "st_ino") + and hasattr(path_st, "st_ino") + and int(getattr(st, "st_ino", -1)) != int(getattr(path_st, "st_ino", -2)) + ): + return False + except Exception: + return False try: dt = time.localtime(float(getattr(st, "st_mtime", time.time()))) @@ -464,10 +561,14 @@ def register_batch_zip_routes(routes: web.RouteTableDef) -> None: entry["ready"] = ok entry["error"] = error entry["count"] = count - try: - loop.call_soon_threadsafe(entry["event"].set) - except Exception: - pass + event = entry.get("event") + else: + event = None + if isinstance(event, asyncio.Event): + try: + loop.call_soon_threadsafe(event.set) + except Exception: + pass if not ok: try: diff --git a/mjr_am_backend/routes/handlers/db_maintenance.py b/mjr_am_backend/routes/handlers/db_maintenance.py index 80e86d1a..d3c50794 100644 --- a/mjr_am_backend/routes/handlers/db_maintenance.py +++ b/mjr_am_backend/routes/handlers/db_maintenance.py @@ -33,6 +33,62 @@ _DB_MAINTENANCE_ACTIVE = False _DB_MAINT_LOCK = threading.Lock() +def _archive_root_resolved() -> Path: + try: + return _DB_ARCHIVE_DIR.resolve(strict=False) + except Exception: + return _DB_ARCHIVE_DIR + + +def _resolve_archive_source(name: str) -> Path | None: + safe_name = str(name or "").strip() + if not safe_name: + return None + if "/" in safe_name or "\\" in safe_name or "\x00" in safe_name: + return None + try: + src = (_DB_ARCHIVE_DIR / safe_name).resolve(strict=False) + except Exception: + return None + try: + src.relative_to(_archive_root_resolved()) + except Exception: + return None + return src + + +def _basename_list(paths: list[str]) -> str: + names: list[str] = [] + for p in paths or []: + try: + names.append(Path(str(p)).name) + except Exception: + continue + return ", ".join(names) + + +def _task_done_logger(label: str): + def _done(task: asyncio.Task) -> None: + try: + exc = task.exception() + except asyncio.CancelledError: + return + except Exception: + return + if exc is not None: + logger.warning("Background task '%s' failed: %s", label, safe_error_message(exc, "task failed")) + + return _done + + +def _spawn_background_task(coro, *, label: str) -> None: + try: + task = asyncio.create_task(coro) + task.add_done_callback(_task_done_logger(label)) + except Exception as exc: + logger.debug("Unable to start background task '%s': %s", label, exc) + + def is_db_maintenance_active() -> bool: with _DB_MAINT_LOCK: return bool(_DB_MAINTENANCE_ACTIVE) @@ -89,7 +145,6 @@ def _list_backup_files() -> list[dict[str, str | int]]: rows.append( { "name": p.name, - "path": str(p), "size_bytes": int(st.st_size), "mtime": int(st.st_mtime), } @@ -323,8 +378,9 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: try: base_path = str(Path(get_runtime_output_root()).resolve(strict=False)) _emit_restore_status("restarting_scan", "info", operation="delete_db") - asyncio.create_task( - index_service.scan_directory(base_path, recursive=True, incremental=False, source="output") + _spawn_background_task( + index_service.scan_directory(base_path, recursive=True, incremental=False, source="output"), + label="db_force_delete_scan_output", ) started_scans.append(base_path) except Exception: @@ -332,8 +388,9 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: try: import folder_paths # type: ignore input_path = str(Path(folder_paths.get_input_directory()).resolve()) - asyncio.create_task( - index_service.scan_directory(input_path, recursive=True, incremental=False, source="input") + _spawn_background_task( + index_service.scan_directory(input_path, recursive=True, incremental=False, source="input"), + label="db_force_delete_scan_input", ) started_scans.append(input_path) except Exception: @@ -383,15 +440,16 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: if failed_files: logger.error("Force-delete: could not remove files: %s", failed_files) + failed_basenames = _basename_list(failed_files) _emit_restore_status( "failed", "error", - f"Could not delete: {', '.join(failed_files)}", + f"Could not delete: {failed_basenames}", operation="delete_db", ) return _json_response(Result.Err( "DELETE_FAILED", - f"Could not delete: {', '.join(failed_files)}. " + f"Could not delete: {failed_basenames}. " "Stop ComfyUI, manually delete the files, then restart.", )) @@ -418,8 +476,9 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: try: base_path = str(Path(get_runtime_output_root()).resolve(strict=False)) _emit_restore_status("restarting_scan", "info", operation="delete_db") - asyncio.create_task( - index_service.scan_directory(base_path, recursive=True, incremental=False, source="output") + _spawn_background_task( + index_service.scan_directory(base_path, recursive=True, incremental=False, source="output"), + label="db_force_delete_scan_output", ) started_scans.append(base_path) except Exception: @@ -427,8 +486,9 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: try: import folder_paths # type: ignore input_path = str(Path(folder_paths.get_input_directory()).resolve()) - asyncio.create_task( - index_service.scan_directory(input_path, recursive=True, incremental=False, source="input") + _spawn_background_task( + index_service.scan_directory(input_path, recursive=True, incremental=False, source="input"), + label="db_force_delete_scan_input", ) started_scans.append(input_path) except Exception: @@ -527,7 +587,7 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: try: rows = _list_backup_files() latest = rows[0]["name"] if rows else None - return _json_response(Result.Ok({"archive_dir": str(_DB_ARCHIVE_DIR), "items": rows, "latest": latest})) + return _json_response(Result.Ok({"archive_dir": _DB_ARCHIVE_DIR.name, "items": rows, "latest": latest})) except Exception as exc: return _json_response(Result.Err("DB_ERROR", safe_error_message(exc, "Failed to list DB backups"))) @@ -564,9 +624,8 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: Result.Ok( { "saved": True, - "archive_dir": str(_DB_ARCHIVE_DIR), + "archive_dir": _DB_ARCHIVE_DIR.name, "name": target.name, - "path": str(target), "size_bytes": int(target.stat().st_size) if target.exists() else 0, } ) @@ -602,7 +661,9 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: chosen = next((x for x in items if str(x.get("name")) == requested_name), {}) if not chosen: return _json_response(Result.Err("NOT_FOUND", f"Backup not found: {requested_name}")) - src = _DB_ARCHIVE_DIR / str(chosen.get("name") or "") + src = _resolve_archive_source(str(chosen.get("name") or "")) + if src is None: + return _json_response(Result.Err("INVALID_INPUT", "Invalid backup name")) if not src.exists(): return _json_response(Result.Err("NOT_FOUND", f"Backup file missing: {src.name}")) except Exception as exc: @@ -654,14 +715,20 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: _emit_restore_status("restarting_scan", "info", operation="restore_db") try: out_path = str(Path(get_runtime_output_root()).resolve(strict=False)) - asyncio.create_task(index_service.scan_directory(out_path, recursive=True, incremental=False, source="output")) + _spawn_background_task( + index_service.scan_directory(out_path, recursive=True, incremental=False, source="output"), + label="db_restore_scan_output", + ) scans_triggered.append(out_path) except Exception: pass try: import folder_paths # type: ignore input_path = str(Path(folder_paths.get_input_directory()).resolve()) - asyncio.create_task(index_service.scan_directory(input_path, recursive=True, incremental=False, source="input")) + _spawn_background_task( + index_service.scan_directory(input_path, recursive=True, incremental=False, source="input"), + label="db_restore_scan_input", + ) scans_triggered.append(input_path) except Exception: pass @@ -680,7 +747,6 @@ def register_db_maintenance_routes(routes: web.RouteTableDef) -> None: { "restored": True, "name": src.name, - "path": str(src), "scans_triggered": scans_triggered if "scans_triggered" in locals() else [], "steps": [ "stopping_workers", diff --git a/mjr_am_backend/routes/handlers/filesystem.py b/mjr_am_backend/routes/handlers/filesystem.py index 587074fe..1a7c5b2f 100644 --- a/mjr_am_backend/routes/handlers/filesystem.py +++ b/mjr_am_backend/routes/handlers/filesystem.py @@ -338,7 +338,11 @@ async def _run_scan_directory_task(svc: dict[str, Any], task: dict[str, Any]) -> def _record_scan_failure(directory: str, source: str, code: str, error: str) -> None: - # No lock needed: called on the asyncio event loop (single-threaded task context). + try: + asyncio.get_running_loop() + except RuntimeError: + logger.warning("Ignoring background scan failure update outside event loop thread") + return _BACKGROUND_SCAN_FAILURES.insert(0, { "timestamp": datetime.datetime.now(datetime.timezone.utc).isoformat(), "directory": directory, diff --git a/mjr_am_backend/routes/handlers/health.py b/mjr_am_backend/routes/handlers/health.py index abb8ae6b..04a2949f 100644 --- a/mjr_am_backend/routes/handlers/health.py +++ b/mjr_am_backend/routes/handlers/health.py @@ -2,6 +2,9 @@ Health check endpoints. """ import asyncio +import hashlib +import os +import re from pathlib import Path from aiohttp import web @@ -23,7 +26,7 @@ from mjr_am_backend.config import ( get_tool_paths, ) from mjr_am_backend.custom_roots import resolve_custom_root -from mjr_am_backend.shared import ErrorCode, Result, sanitize_error_message +from mjr_am_backend.shared import ErrorCode, Result, get_logger, sanitize_error_message from mjr_am_backend.tool_detect import get_tool_status from mjr_am_backend.utils import parse_bool @@ -37,6 +40,7 @@ from ..core import ( _require_services, _require_write_access, ) +from ..core.security import _refresh_trusted_proxy_cache, _request_transport_is_secure, _safe_mode_enabled from .db_maintenance import is_db_maintenance_active SECURITY_PREF_KEYS = { @@ -49,11 +53,103 @@ SECURITY_PREF_KEYS = { "allow_reset_index", "api_token", } +_VALID_PROBE_MODES = {"auto", "exiftool", "ffprobe", "both"} +_CUSTOM_ROOT_ID_RE = re.compile(r"^[A-Za-z0-9._-]{1,128}$") +_WRITE_TOKEN_COOKIE_NAME = "mjr_write_token" +logger = get_logger(__name__) def _extract_probe_mode(body: dict) -> str: raw_mode = body.get("mode") or body.get("media_probe_backend") or "" - return str(raw_mode).strip() + mode = str(raw_mode).strip().lower() + return mode if mode in _VALID_PROBE_MODES else "" + + +def _hash_api_token(token: str) -> str: + try: + normalized = str(token or "").strip() + except Exception: + normalized = "" + try: + pepper = str(os.environ.get("MAJOOR_API_TOKEN_PEPPER") or "").strip() + except Exception: + pepper = "" + payload = f"{pepper}\0{normalized}".encode("utf-8", errors="ignore") + return hashlib.sha256(payload).hexdigest() + + +def _is_valid_custom_root_id(value: object) -> bool: + try: + return bool(_CUSTOM_ROOT_ID_RE.match(str(value or ""))) + except Exception: + return False + + +def _is_secure_request_transport(request: web.Request) -> bool: + try: + peer = str(getattr(request, "remote", "") or "").strip() + except Exception: + peer = "" + if not peer: + try: + peer = str(getattr(request.transport, "get_extra_info", lambda *_args, **_kwargs: None)("peername") or "") + except Exception: + peer = "" + try: + scheme = str(getattr(request, "scheme", "") or "").strip().lower() + except Exception: + scheme = "" + try: + headers = request.headers + except Exception: + headers = {} + return bool(_request_transport_is_secure(peer_ip=peer, headers=headers, request_scheme=scheme)) + + +def _bootstrap_enabled() -> bool: + try: + raw = str(os.environ.get("MAJOOR_ALLOW_BOOTSTRAP") or "").strip().lower() + except Exception: + raw = "" + return raw in {"1", "true", "yes", "on"} + + +def _should_expose_token_response() -> bool: + try: + raw = str(os.environ.get("MAJOOR_EXPOSE_TOKEN_IN_RESPONSE") or "").strip().lower() + except Exception: + raw = "" + return raw in {"1", "true", "yes", "on"} + + +def _token_hint(token: object) -> str: + normalized = str(token or "").strip() + if not normalized: + return "" + tail = normalized[-4:] if len(normalized) >= 4 else normalized + return f"...{tail}" + + +def _set_write_token_cookie(response: web.StreamResponse, request: web.Request, token: str) -> None: + normalized = str(token or "").strip() + if not normalized: + return + try: + scheme = str(getattr(request, "scheme", "") or "").strip().lower() + except Exception: + scheme = "" + secure_cookie = scheme == "https" + try: + response.set_cookie( + _WRITE_TOKEN_COOKIE_NAME, + normalized, + httponly=True, + samesite="Strict", + secure=secure_cookie, + path="/", + ) + except Exception: + return def _extract_metadata_fallback_payload(body: dict) -> tuple[object | None, object | None]: @@ -73,11 +169,15 @@ def _build_security_prefs(body: dict) -> dict[str, object]: if key not in body: continue if key == "api_token": - prefs[key] = str(body[key] or "").strip() + token = str(body[key] or "").strip() + if token: + prefs["api_token_hash"] = _hash_api_token(token) else: prefs[key] = parse_bool(body[key], False) - if "apiToken" in body and "api_token" not in prefs: - prefs["api_token"] = str(body.get("apiToken") or "").strip() + if "apiToken" in body and "api_token_hash" not in prefs: + token = str(body.get("apiToken") or "").strip() + if token: + prefs["api_token_hash"] = _hash_api_token(token) return prefs @@ -168,6 +268,8 @@ def register_health_routes(routes: web.RouteTableDef) -> None: str(Path(folder_paths.get_input_directory()).resolve(strict=False)), ] elif scope == "custom": + if not _is_valid_custom_root_id(custom_root_id): + return _json_response(Result.Err(ErrorCode.INVALID_INPUT, "Invalid custom_root_id")) root_result = resolve_custom_root(str(custom_root_id or "")) if not root_result.ok: return _json_response(Result.Err(ErrorCode.INVALID_INPUT, root_result.error)) @@ -355,6 +457,14 @@ def register_health_routes(routes: web.RouteTableDef) -> None: body = body_res.data or {} raw_value = body.get("output_directory") value = "" if raw_value is None else str(raw_value).strip() + if value: + try: + normalized_path = Path(value).expanduser().resolve(strict=True) + except Exception: + return _json_response(Result.Err("INVALID_INPUT", "output_directory must be an existing directory")) + if not normalized_path.is_dir(): + return _json_response(Result.Err("INVALID_INPUT", "output_directory must be a directory")) + value = str(normalized_path) result = await settings_service.set_output_directory(value) if not result.ok: return _json_response(result) @@ -478,6 +588,14 @@ def register_health_routes(routes: web.RouteTableDef) -> None: result = await settings_service.set_security_prefs(prefs) if result.ok: + try: + _safe_mode_enabled.cache_clear() + except Exception: + pass + try: + _refresh_trusted_proxy_cache() + except Exception: + pass current_prefs = result.data or (await settings_service.get_security_prefs()) return _json_response(Result.Ok({"prefs": current_prefs})) return _json_response(result) @@ -499,24 +617,68 @@ def register_health_routes(routes: web.RouteTableDef) -> None: settings_service = svc.get("settings") if not settings_service: return _json_response(Result.Err("SERVICE_UNAVAILABLE", "Settings service unavailable")) + if not _is_secure_request_transport(request): + return _json_response( + Result.Err( + "FORBIDDEN", + "Token rotation response is only allowed over HTTPS or loopback transport.", + ) + ) result = await settings_service.rotate_api_token() if not result.ok: return _json_response(result) - return _json_response(Result.Ok({"token": (result.data or {}).get("api_token", "")})) + token = str((result.data or {}).get("api_token") or "").strip() + if _is_loopback_request(request): + try: + scheme = str(getattr(request, "scheme", "") or "").strip().lower() + except Exception: + scheme = "" + if scheme != "https": + logger.warning("Token rotation requested over plain HTTP loopback transport.") + payload = {"token_hint": _token_hint(token)} + if token and _should_expose_token_response(): + payload["token"] = token + response = _json_response(Result.Ok(payload)) + _set_write_token_cookie(response, request, token) + return response @routes.post("/mjr/am/settings/security/bootstrap-token") async def bootstrap_security_token(request): csrf = _csrf_error(request) if csrf: return _json_response(Result.Err("CSRF", csrf)) + + is_loopback = _is_loopback_request(request) + + # Remote requests must explicitly opt-in via MAJOOR_ALLOW_BOOTSTRAP=1. + # Loopback is always allowed: only local processes can reach it, and the + # auto-generated session token must be deliverable without user configuration. + if not is_loopback and not _bootstrap_enabled(): + return _json_response( + Result.Err( + "BOOTSTRAP_DISABLED", + "Bootstrap token is disabled. Set MAJOOR_ALLOW_BOOTSTRAP=1 for initial token provisioning.", + ) + ) + auth = _require_write_access(request) if not auth.ok: - user_auth = _require_authenticated_user(request) - if not (user_auth.ok and _is_loopback_request(request)): + if not is_loopback: return _json_response(auth) + user_auth = _require_authenticated_user(request) + auth_mode = str((user_auth.meta or {}).get("auth_mode") or "").strip().lower() + if not (user_auth.ok and auth_mode == "comfy_user"): + return _json_response( + Result.Err( + "AUTH_REQUIRED", + "Bootstrap requires an authenticated ComfyUI user on loopback when API token auth is unavailable.", + ) + ) - if _has_configured_write_token(): + # Remote: block when a persistent token is already configured (use rotate-token instead). + # Loopback: always deliver the session token — the user never configured it manually. + if not is_loopback and _has_configured_write_token(): return _json_response( Result.Err( "FORBIDDEN", @@ -531,11 +693,34 @@ def register_health_routes(routes: web.RouteTableDef) -> None: settings_service = svc.get("settings") if not settings_service: return _json_response(Result.Err("SERVICE_UNAVAILABLE", "Settings service unavailable")) + if not _is_secure_request_transport(request): + return _json_response( + Result.Err( + "FORBIDDEN", + "Token bootstrap response is only allowed over HTTPS or loopback transport.", + ) + ) result = await settings_service.bootstrap_api_token() if not result.ok: return _json_response(result) - return _json_response(Result.Ok({"token": (result.data or {}).get("api_token", "")})) + token = str((result.data or {}).get("api_token") or "").strip() + if is_loopback: + try: + scheme = str(getattr(request, "scheme", "") or "").strip().lower() + except Exception: + scheme = "" + if scheme != "https": + logger.warning("Token bootstrap requested over plain HTTP loopback transport.") + payload = {"token_hint": _token_hint(token)} + # Include plain token in body for loopback: only local processes can reach loopback, + # so returning the token in the JSON body is safe and allows the frontend to cache + # it in sessionStorage without any user action. + if token and (is_loopback or _should_expose_token_response()): + payload["token"] = token + response = _json_response(Result.Ok(payload)) + _set_write_token_cookie(response, request, token) + return response @routes.get("/mjr/am/tools/status") async def tools_status(request): diff --git a/mjr_am_backend/routes/handlers/releases.py b/mjr_am_backend/routes/handlers/releases.py index 7f4c173d..1962a778 100644 --- a/mjr_am_backend/routes/handlers/releases.py +++ b/mjr_am_backend/routes/handlers/releases.py @@ -14,6 +14,7 @@ from __future__ import annotations import asyncio import os +import re from typing import Any from aiohttp import ClientSession, ClientTimeout, web @@ -22,6 +23,7 @@ from mjr_am_backend.shared import Result, get_logger, sanitize_error_message from ..core import _json_response logger = get_logger(__name__) +_SAFE_GITHUB_SEGMENT_RE = re.compile(r"^[A-Za-z0-9](?:[A-Za-z0-9._-]{0,98}[A-Za-z0-9])?$") def _parse_per_page(raw_value: str | None) -> int: @@ -32,6 +34,20 @@ def _parse_per_page(raw_value: str | None) -> int: return max(1, min(200, value)) +def _is_safe_github_segment(value: str) -> bool: + try: + normalized = str(value or "").strip() + except Exception: + return False + if not normalized: + return False + if ".." in normalized: + return False + if normalized.startswith(".") or normalized.endswith("."): + return False + return bool(_SAFE_GITHUB_SEGMENT_RE.match(normalized)) + + def _github_headers() -> dict[str, str]: token = os.environ.get("GITHUB_TOKEN") or os.environ.get("MAJOOR_GITHUB_TOKEN") headers = {"Accept": "application/vnd.github.v3+json"} @@ -61,8 +77,7 @@ def _extract_ref_names(payload: Any) -> list[str]: async def _fetch_github_json(session: ClientSession, url: str, headers: dict[str, str]) -> Any: async with session.get(url, headers=headers, timeout=ClientTimeout(total=30)) as resp: if resp.status != 200: - text = await resp.text() - raise RuntimeError(f"GitHub API returned {resp.status}: {text}") + raise RuntimeError(f"GitHub API returned {resp.status}") return await resp.json() @@ -71,6 +86,8 @@ def register_releases_routes(routes: web.RouteTableDef) -> None: async def get_releases(request: web.Request) -> web.Response: owner = (request.query.get("owner") or "MajoorWaldi").strip() repo = (request.query.get("repo") or "ComfyUI-Majoor-AssetsManager").strip() + if not _is_safe_github_segment(owner) or not _is_safe_github_segment(repo): + return _json_response(Result.Err("INVALID_INPUT", "Invalid owner/repo format")) per_page = _parse_per_page(request.query.get("per_page")) headers = _github_headers() tags_url, branches_url = _github_refs_urls(owner, repo, per_page) diff --git a/mjr_am_backend/routes/handlers/scan_helpers.py b/mjr_am_backend/routes/handlers/scan_helpers.py index 8a00ec8f..698a3b50 100644 --- a/mjr_am_backend/routes/handlers/scan_helpers.py +++ b/mjr_am_backend/routes/handlers/scan_helpers.py @@ -22,7 +22,8 @@ logger = get_logger(__name__) _DEFAULT_MAX_UPLOAD_SIZE_BYTES = 500 * 1024 * 1024 try: _DB_CONSISTENCY_SAMPLE = int(os.environ.get("MJR_DB_CONSISTENCY_SAMPLE", "32")) -except Exception: +except Exception as exc: + logger.warning("Invalid MJR_DB_CONSISTENCY_SAMPLE value; using default 32 (%s)", exc) _DB_CONSISTENCY_SAMPLE = 32 _DB_CONSISTENCY_COOLDOWN_SECONDS = env_float("MJR_DB_CONSISTENCY_COOLDOWN_SECONDS", 3600.0) _DEFAULT_MAX_RENAME_ATTEMPTS = 1000 @@ -193,6 +194,7 @@ async def _write_multipart_file_atomic(dest_dir: Path, filename: str, field) -> fd = None tmp_path = None + claimed_final: Path | None = None try: fd, tmp_path = tempfile.mkstemp(dir=str(dest_dir), prefix=".upload_", suffix=ext) with os.fdopen(fd, "wb") as handle: @@ -200,10 +202,15 @@ async def _write_multipart_file_atomic(dest_dir: Path, filename: str, field) -> await _write_upload_chunks(field, handle) tmp_obj = Path(str(tmp_path)) - final = _unique_upload_destination(dest_dir, safe_name, ext) - tmp_obj.replace(final) - return Result.Ok(final) + claimed_final = _unique_upload_destination(dest_dir, safe_name, ext) + tmp_obj.replace(claimed_final) + return Result.Ok(claimed_final) except Exception as exc: + if claimed_final is not None: + try: + claimed_final.unlink(missing_ok=True) + except Exception: + pass _cleanup_temp_upload_file(fd, tmp_path) return Result.Err("UPLOAD_FAILED", safe_error_message(exc, "Upload failed")) @@ -229,14 +236,18 @@ async def _write_upload_chunks(field, handle) -> None: def _unique_upload_destination(dest_dir: Path, safe_name: str, ext: str) -> Path: - final = dest_dir / safe_name counter = 0 - while final.exists(): - counter += 1 - if counter > _MAX_RENAME_ATTEMPTS: - raise RuntimeError("Too many rename attempts") - final = dest_dir / f"{Path(safe_name).stem}_{counter}{ext}" - return final + while True: + final = dest_dir / safe_name if counter == 0 else dest_dir / f"{Path(safe_name).stem}_{counter}{ext}" + try: + # Atomically claim the destination to avoid TOCTOU races across concurrent uploads. + with final.open("xb"): + pass + return final + except FileExistsError: + counter += 1 + if counter > _MAX_RENAME_ATTEMPTS: + raise RuntimeError("Too many rename attempts") from None def _cleanup_temp_upload_file(fd: int | None, tmp_path: str | None) -> None: diff --git a/mjr_am_backend/routes/handlers/scan_upload.py b/mjr_am_backend/routes/handlers/scan_upload.py index 3394399f..18f9c693 100644 --- a/mjr_am_backend/routes/handlers/scan_upload.py +++ b/mjr_am_backend/routes/handlers/scan_upload.py @@ -24,6 +24,7 @@ from ..core import ( _require_write_access, ) from .scan_helpers import ( + _MAX_UPLOAD_SIZE, _read_upload_file_field, _schedule_index_task, _upload_skip_index, @@ -84,6 +85,14 @@ def register_upload_routes(routes: web.RouteTableDef, *, deps: dict | None = Non auth = _require_write_access_(request) if not auth.ok: return _json_response(auth) + try: + content_length = int(request.content_length or 0) + except Exception: + content_length = 0 + if content_length > int(_MAX_UPLOAD_SIZE): + return _json_response( + Result.Err("FILE_TOO_LARGE", f"Upload exceeds {_MAX_UPLOAD_SIZE} bytes") + ) try: field, filename, upload_err = await _read_upload_file_field(request) diff --git a/mjr_am_backend/settings.py b/mjr_am_backend/settings.py index df1ae062..7cb7f5c8 100644 --- a/mjr_am_backend/settings.py +++ b/mjr_am_backend/settings.py @@ -132,22 +132,18 @@ class AppSettings: token_hash = await self._get_stored_api_token_hash_locked() if token_hash: - stored_token = await self._get_stored_api_token_plain_locked(token_hash) - if stored_token: - self._set_runtime_api_token(stored_token, token_hash) - self._set_api_token_env(stored_token, token_hash, include_plain=False) - return stored_token - - token = self._generate_api_token() - new_hash = await self._persist_api_token_hash_with_warning(token, "Failed to persist regenerated API token hash") - self._set_runtime_api_token(token, new_hash) - self._set_api_token_env(token, new_hash, include_plain=False) - return token + self._set_runtime_api_token("", token_hash) + self._set_api_token_env("", token_hash, include_plain=False) + return "" + # Auto-generate a session token. + # Not persisted to DB, not injected into env — this keeps loopback connections + # accessible without any token by default (no user action required). + # The token is held in memory only and delivered to the frontend via the + # bootstrap endpoint on loopback, which caches it in sessionStorage. token = self._generate_api_token() - token_hash = await self._persist_api_token_hash_with_warning(token, "Failed to persist auto-generated API token hash") + token_hash = self._hash_api_token(token) self._set_runtime_api_token(token, token_hash) - self._set_api_token_env(token, token_hash, include_plain=False) return token def _env_api_token(self) -> str: @@ -161,9 +157,6 @@ class AppSettings: write_res = await self._write_setting(_SECURITY_API_TOKEN_HASH_KEY, token_hash) if not write_res.ok: logger.warning("%s: %s", warning_message, write_res.error) - plain_res = await self._write_setting(_SECURITY_API_TOKEN_KEY, str(token or "").strip()) - if not plain_res.ok: - logger.warning("%s (plain token cache): %s", warning_message, plain_res.error) return token_hash def _set_runtime_api_token(self, token: str, token_hash: str) -> None: @@ -236,13 +229,18 @@ class AppSettings: async def set_security_prefs(self, prefs: Mapping[str, Any]) -> Result[dict[str, Any]]: to_write = self._extract_security_prefs_to_write(prefs) token_in_payload = self._extract_token_from_prefs_payload(prefs) - if not to_write and token_in_payload is None: + token_hash_in_payload = self._extract_token_hash_from_prefs_payload(prefs) + if not to_write and token_in_payload is None and token_hash_in_payload is None: return Result.Err("INVALID_INPUT", "No security settings provided") async with self._lock: write_err = await self._persist_security_pref_flags(to_write) if write_err is not None: return write_err - if token_in_payload is not None: + if token_hash_in_payload is not None: + token_err = await self._persist_security_api_token_hash(token_hash_in_payload) + if token_err is not None: + return token_err + elif token_in_payload is not None: token_err = await self._persist_security_api_token(token_in_payload) if token_err is not None: return token_err @@ -263,6 +261,19 @@ class AppSettings: return prefs.get("apiToken") return None + def _extract_token_hash_from_prefs_payload(self, prefs: Mapping[str, Any]) -> str | None: + if not isinstance(prefs, Mapping): + return None + raw = prefs.get("api_token_hash") + if raw is None: + return None + value = str(raw or "").strip().lower() + if len(value) != 64: + return None + if any(ch not in "0123456789abcdef" for ch in value): + return None + return value + async def _persist_security_pref_flags(self, to_write: Mapping[str, bool]) -> Result[dict[str, Any]] | None: for key, value in to_write.items(): res = await self._write_setting(key, "1" if value else "0") @@ -286,13 +297,21 @@ class AppSettings: hash_res = await self._write_setting(_SECURITY_API_TOKEN_HASH_KEY, token_hash) if not hash_res.ok: return Result.Err("DB_ERROR", hash_res.error or "Failed to persist api_token") - plain_res = await self._write_setting(_SECURITY_API_TOKEN_KEY, token) - if not plain_res.ok: - return Result.Err("DB_ERROR", plain_res.error or "Failed to persist api_token") self._set_runtime_api_token(token, token_hash) self._set_api_token_env(token, token_hash, include_plain=False) return None + async def _persist_security_api_token_hash(self, token_hash: str) -> Result[dict[str, Any]] | None: + value = str(token_hash or "").strip().lower() + if len(value) != 64 or any(ch not in "0123456789abcdef" for ch in value): + return Result.Err("INVALID_INPUT", "Invalid api_token_hash") + hash_res = await self._write_setting(_SECURITY_API_TOKEN_HASH_KEY, value) + if not hash_res.ok: + return Result.Err("DB_ERROR", hash_res.error or "Failed to persist api_token_hash") + self._set_runtime_api_token("", value) + self._set_api_token_env("", value, include_plain=False) + return None + async def _warn_if_bump_fails(self, message: str) -> None: bump = await self._bump_settings_version_locked() if not bump.ok: @@ -308,9 +327,6 @@ class AppSettings: hash_res = await self._write_setting(_SECURITY_API_TOKEN_HASH_KEY, token_hash) if not hash_res.ok: return Result.Err("DB_ERROR", hash_res.error or "Failed to persist rotated api token") - plain_res = await self._write_setting(_SECURITY_API_TOKEN_KEY, token) - if not plain_res.ok: - return Result.Err("DB_ERROR", plain_res.error or "Failed to persist rotated api token") self._set_runtime_api_token(token, token_hash) self._set_api_token_env(token, token_hash, include_plain=False) bump = await self._bump_settings_version_locked() diff --git a/mjr_am_backend/tool_detect.py b/mjr_am_backend/tool_detect.py index 753fb353..d0967c88 100644 --- a/mjr_am_backend/tool_detect.py +++ b/mjr_am_backend/tool_detect.py @@ -5,6 +5,7 @@ Cached detection to avoid repeated subprocess calls. import re import shutil import subprocess +import threading from typing import Any from mjr_am_backend.config import ( @@ -28,6 +29,7 @@ _TOOL_VERSIONS: dict[str, str | None] = { "exiftool": None, "ffprobe": None, } +_TOOL_CACHE_LOCK = threading.Lock() def parse_tool_version(value: str | None) -> tuple[int, ...]: @@ -84,71 +86,73 @@ def _run_command(cmd: list[str]) -> subprocess.CompletedProcess[str]: def has_exiftool() -> bool: """Check if ExifTool is available.""" - if _TOOL_CACHE["exiftool"] is not None: - return _TOOL_CACHE["exiftool"] + with _TOOL_CACHE_LOCK: + if _TOOL_CACHE["exiftool"] is not None: + return bool(_TOOL_CACHE["exiftool"]) - try: - exiftool_bin = EXIFTOOL_BIN or "exiftool" - if shutil.which(exiftool_bin) is None: - logger.debug("ExifTool binary not found in PATH: %s", exiftool_bin) - result = _run_command([exiftool_bin, "-ver"]) - available = result.returncode == 0 - _TOOL_CACHE["exiftool"] = available + try: + exiftool_bin = EXIFTOOL_BIN or "exiftool" + if shutil.which(exiftool_bin) is None: + logger.debug("ExifTool binary not found in PATH: %s", exiftool_bin) + result = _run_command([exiftool_bin, "-ver"]) + available = result.returncode == 0 + _TOOL_CACHE["exiftool"] = available - if available: - version = result.stdout.strip() - _TOOL_VERSIONS["exiftool"] = version - if not _enforce_min_version("ExifTool", version, EXIFTOOL_MIN_VERSION): - _TOOL_CACHE["exiftool"] = False - return False - logger.info("ExifTool detected: version %s", version) - else: - logger.warning("ExifTool not found or failed to start: %s", result.stderr.strip()) + if available: + version = result.stdout.strip() + _TOOL_VERSIONS["exiftool"] = version + if not _enforce_min_version("ExifTool", version, EXIFTOOL_MIN_VERSION): + _TOOL_CACHE["exiftool"] = False + return False + logger.info("ExifTool detected: version %s", version) + else: + logger.warning("ExifTool not found or failed to start: %s", result.stderr.strip()) - return available - except (FileNotFoundError, subprocess.TimeoutExpired) as exc: - logger.warning("ExifTool detection failed: %s", exc) - _TOOL_CACHE["exiftool"] = False - return False - except Exception as exc: - logger.warning("ExifTool detection raised unexpected error: %s", exc) - _TOOL_CACHE["exiftool"] = False - return False + return available + except (FileNotFoundError, subprocess.TimeoutExpired) as exc: + logger.warning("ExifTool detection failed: %s", exc) + _TOOL_CACHE["exiftool"] = False + return False + except Exception as exc: + logger.warning("ExifTool detection raised unexpected error: %s", exc) + _TOOL_CACHE["exiftool"] = False + return False def has_ffprobe() -> bool: """Check if FFprobe is available.""" - if _TOOL_CACHE["ffprobe"] is not None: - return _TOOL_CACHE["ffprobe"] + with _TOOL_CACHE_LOCK: + if _TOOL_CACHE["ffprobe"] is not None: + return bool(_TOOL_CACHE["ffprobe"]) - try: - ffprobe_bin = FFPROBE_BIN or "ffprobe" - if shutil.which(ffprobe_bin) is None: - logger.debug("FFprobe binary not found in PATH: %s", ffprobe_bin) - result = _run_command([ffprobe_bin, "-version"]) - available = result.returncode == 0 - _TOOL_CACHE["ffprobe"] = available + try: + ffprobe_bin = FFPROBE_BIN or "ffprobe" + if shutil.which(ffprobe_bin) is None: + logger.debug("FFprobe binary not found in PATH: %s", ffprobe_bin) + result = _run_command([ffprobe_bin, "-version"]) + available = result.returncode == 0 + _TOOL_CACHE["ffprobe"] = available - if available: - version_line = result.stdout.split("\n")[0] if result.stdout else "" - version = version_line.strip() - _TOOL_VERSIONS["ffprobe"] = version - if not _enforce_min_version("FFprobe", version, FFPROBE_MIN_VERSION): - _TOOL_CACHE["ffprobe"] = False - return False - logger.info("FFprobe detected: %s", version) - else: - logger.warning("FFprobe not found or failed to start: %s", result.stderr.strip()) + if available: + version_line = result.stdout.split("\n")[0] if result.stdout else "" + version = version_line.strip() + _TOOL_VERSIONS["ffprobe"] = version + if not _enforce_min_version("FFprobe", version, FFPROBE_MIN_VERSION): + _TOOL_CACHE["ffprobe"] = False + return False + logger.info("FFprobe detected: %s", version) + else: + logger.warning("FFprobe not found or failed to start: %s", result.stderr.strip()) - return available - except (FileNotFoundError, subprocess.TimeoutExpired) as exc: - logger.warning("FFprobe detection failed: %s", exc) - _TOOL_CACHE["ffprobe"] = False - return False - except Exception as exc: - logger.warning("FFprobe detection raised unexpected error: %s", exc) - _TOOL_CACHE["ffprobe"] = False - return False + return available + except (FileNotFoundError, subprocess.TimeoutExpired) as exc: + logger.warning("FFprobe detection failed: %s", exc) + _TOOL_CACHE["ffprobe"] = False + return False + except Exception as exc: + logger.warning("FFprobe detection raised unexpected error: %s", exc) + _TOOL_CACHE["ffprobe"] = False + return False def get_tool_status() -> dict[str, Any]: @@ -176,5 +180,6 @@ def get_tool_status() -> dict[str, Any]: def reset_tool_cache(): """Reset tool detection cache (for testing or manual refresh).""" global _TOOL_CACHE, _TOOL_VERSIONS - _TOOL_CACHE = {"exiftool": None, "ffprobe": None} - _TOOL_VERSIONS = {"exiftool": None, "ffprobe": None} + with _TOOL_CACHE_LOCK: + _TOOL_CACHE = {"exiftool": None, "ffprobe": None} + _TOOL_VERSIONS = {"exiftool": None, "ffprobe": None} diff --git a/mjr_am_shared/errors.py b/mjr_am_shared/errors.py index beb62744..8db7c60c 100644 --- a/mjr_am_shared/errors.py +++ b/mjr_am_shared/errors.py @@ -13,7 +13,7 @@ logger = get_logger(__name__) _DEBUG_MODE = os.getenv("MJR_DEBUG", "").strip().lower() in ("1", "true", "yes", "on") _WINDOWS_PATH_RE = re.compile(r"[A-Za-z]:\\[^\s]+") _UNC_PATH_RE = re.compile(r"\\\\[^\s\\]+\\[^\s]+") -_UNIX_PATH_RE = re.compile(r"/[^\s]+") +_UNIX_PATH_RE = re.compile(r"(? str: diff --git a/mjr_am_shared/log.py b/mjr_am_shared/log.py index 08b10736..32f0668a 100644 --- a/mjr_am_shared/log.py +++ b/mjr_am_shared/log.py @@ -53,6 +53,22 @@ class EmojiFormatter(logging.Formatter): formatter = logging.Formatter(log_format) return formatter.format(record) + +def _has_correlation_filter(logger: logging.Logger) -> bool: + try: + return any(isinstance(f, CorrelationFilter) for f in list(logger.filters or [])) + except Exception: + return False + + +def _ensure_correlation_filter(logger: logging.Logger) -> None: + if _has_correlation_filter(logger): + return + try: + logger.addFilter(CorrelationFilter()) + except Exception: + pass + def get_logger(name: str, level: int | None = None) -> logging.Logger: """ Get a logger with Majoor prefix and emoji indicators. @@ -77,10 +93,7 @@ def get_logger(name: str, level: int | None = None) -> logging.Logger: name = ".".join(parts[idx:]) logger = logging.getLogger(f"majoor.{name}") - try: - logger.addFilter(CorrelationFilter()) - except Exception: - pass + _ensure_correlation_filter(logger) if level is not None: logger.setLevel(level) diff --git a/mjr_am_shared/scan_throttle.py b/mjr_am_shared/scan_throttle.py index 9ad85860..7e4816ff 100644 --- a/mjr_am_shared/scan_throttle.py +++ b/mjr_am_shared/scan_throttle.py @@ -108,3 +108,12 @@ def _cleanup_locked(now: float) -> None: keys_to_remove = [k for k, v in _RECENT_SCAN_TIMES.items() if v < cutoff] for k in keys_to_remove: _RECENT_SCAN_TIMES.pop(k, None) + + +def _reset_scan_throttle_state_for_tests() -> None: + """ + Test helper: clear all in-memory scan throttle state. + """ + with _LOCK: + _MANUAL_SCAN_TIMES.clear() + _RECENT_SCAN_TIMES.clear() diff --git a/mjr_am_shared/types.py b/mjr_am_shared/types.py index b5a63fbb..feff64a4 100644 --- a/mjr_am_shared/types.py +++ b/mjr_am_shared/types.py @@ -1,6 +1,7 @@ """ Shared types, enums, and constants. """ +import os from enum import Enum from typing import Final, Literal @@ -58,7 +59,6 @@ def classify_file(filename: str) -> FileKind: Returns: File kind (image, video, audio, model3d, unknown) """ - import os ext = os.path.splitext(filename)[1].lower() for kind, exts in EXTENSIONS.items(): diff --git a/mjr_am_shared/version.py b/mjr_am_shared/version.py index 8108e780..297cd238 100644 --- a/mjr_am_shared/version.py +++ b/mjr_am_shared/version.py @@ -3,6 +3,7 @@ from __future__ import annotations import os import re import subprocess +from functools import lru_cache from pathlib import Path from typing import TypedDict @@ -43,7 +44,8 @@ def _resolve_branch_from_env() -> str: return "" -def _run_git(args: list[str]) -> str: +@lru_cache(maxsize=16) +def _run_git(*args: str) -> str: try: root = Path(__file__).resolve().parent.parent result = subprocess.run( @@ -61,7 +63,7 @@ def _run_git(args: list[str]) -> str: def _resolve_branch_from_git() -> str: - branch = _run_git(["rev-parse", "--abbrev-ref", "HEAD"]) + branch = _run_git("rev-parse", "--abbrev-ref", "HEAD") # Detached HEAD commonly returns literal "HEAD". if not branch or branch.upper() == "HEAD": return "" @@ -79,7 +81,7 @@ def _is_nightly_checkout(version: str, branch: str) -> bool: # If running from git and HEAD isn't exactly the stable release tag, # treat it as nightly/development build. - exact_tag = _run_git(["describe", "--tags", "--exact-match"]) + exact_tag = _run_git("describe", "--tags", "--exact-match") if exact_tag: if _looks_nightly(exact_tag): return True diff --git a/pyproject.toml b/pyproject.toml index 3d78127c..92e80d6b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,21 +25,27 @@ authors = [ # ] dependencies = [ - "aiohttp>=3.8.0", - "aiosqlite>=0.20.0", - "pillow>=10.0.0", - "hachoir>=3.3.0", - "send2trash>=1.8.0", - "pywin32>=300; sys_platform == 'win32'", + "aiohttp>=3.8.0,<4.0.0", + "aiosqlite>=0.20.0,<1.0.0", + "pillow>=10.0.0,<13.0.0", + "hachoir>=3.3.0,<4.0.0", + "send2trash>=1.8.0,<2.0.0", + "watchdog>=3.0.0,<7.0.0", + "pywin32>=300,<311; sys_platform == 'win32'", # Note: tkinter is required for native folder browser functionality # tkinter is included in standard Python distribution but may be missing in minimal installs ] [project.optional-dependencies] dev = [ - "radon>=6.0.1", - "flake8>=7.1.0", - "flake8-cognitive-complexity>=0.1.0", + "pytest>=8.0.0,<9.0.0", + "pytest-asyncio>=0.22.0,<1.0.0", + "pyflakes>=3.4.0,<4.0.0", + "radon>=6.0.1,<7.0.0", + "flake8>=7.1.0,<8.0.0", + "flake8-cognitive-complexity>=0.1.0,<1.0.0", + "pip-audit>=2.9.0,<3.0.0", + "bandit>=1.8.0,<2.0.0", ] [project.urls] diff --git a/pytest.ini b/pytest.ini index d0c4406a..f2c5e368 100644 --- a/pytest.ini +++ b/pytest.ini @@ -2,95 +2,8 @@ # Collect only this repository's tests. testpaths = tests -# Strict minimal suite: only critical smoke/regression tests. -python_files = - test_import_smoke.py - test_routes.py - test_schema_heal.py - test_write_access_guard.py - test_browser_service.py - test_health_service.py - test_collections_service.py - test_duplicates_service.py - test_search_filters.py - test_search_result_filter.py - test_search_result_hydrator.py - test_search_impl_routes.py - test_search_impl_helpers.py - test_health_routes.py - test_misc_routes.py - test_metadata_route_more.py - test_scan_helpers.py - test_scan_routes.py - test_scan_routes_more.py - test_db_maintenance_routes.py - test_db_maintenance_more.py - test_assets_impl_routes.py - test_custom_roots_routes.py - test_custom_roots_module.py - test_extractors.py - test_extractors_helpers.py - test_geninfo_flux.py - test_rating_tags_import.py - test_path_utils.py - test_request_json.py - test_paths_services.py - test_registry_more.py - test_deps.py - test_settings_backend.py - test_exiftool_adapter.py - test_metadata_service_impl.py - test_metadata_small_modules.py - test_metadata_retry_and_parsing_utils.py - test_geninfo_parser_impl_helpers.py - test_geninfo_parser_impl_more.py - test_sqlite_facade_helpers.py - test_sqlite_facade_integration.py - test_sqlite_facade_more.py - test_index_entry_builder.py - test_index_prepare_ops.py - test_index_runtime_helpers.py - test_index_scan_orchestrator.py - test_index_file_ops.py - test_index_service.py - test_index_searcher.py - test_index_watcher.py - test_index_batching_persistence_streaming.py - test_index_fs_walker.py - test_index_fs_walker_more.py - test_index_scan_batch_utils.py - test_index_scan_batch_utils_more.py - test_index_metadata_helpers.py - test_metadata_extractor_registry.py - test_filesystem_helpers.py - test_filesystem_background_more.py - test_collections_duplicates_routes.py - test_batch_zip_routes.py - test_browser_back_navigation_guard.py - test_db_reset_async_safety.py - test_route_security_guards.py - test_security_core_helpers.py - test_assets_path_guard.py - test_db_connection_pool_recovery.py - test_db_refactor_helpers.py - test_probe_router_tool_detect.py - test_routes_compat.py - test_tags_sync.py - test_shared_utils.py - test_scan_throttle.py - test_parser_impl_extra.py - test_index_batching_extra.py - test_index_prepare_ops_extra.py - test_extractors_more.py - test_index_db_ops.py - test_role_classifier.py - test_model_tracer.py - test_index_entry_builder_extra.py - test_scan_diagnostics.py - test_db_recovery_extra.py - test_sampler_tracer_extra.py - test_search_flow_integration.py - test_scan_flow_integration.py +python_files = test_*.py +asyncio_mode = auto norecursedirs = __old_version_zip .* build dist __pycache__ node_modules tests/__pytest_tmp__ tests/__pytest_tmp_* __pytest_tmp__ __pytest_tmp_* addopts = -ra -p no:cacheprovider --basetemp=tests/__pytest_tmp__ diff --git a/requirements.txt b/requirements.txt index 04a42d21..52be76c0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,33 +1,29 @@ # ComfyUI-Majoor-AssetsManager Python Dependencies # Core web framework -aiohttp>=3.8.0 +aiohttp>=3.8.0,<4.0.0 # Async SQLite (non-blocking DB adapter) -aiosqlite>=0.20.0 +aiosqlite>=0.20.0,<1.0.0 # Image processing and thumbnails # Updated for security patches (CVE-2023-50447, CVE-2024-28219) -pillow>=10.0.0 +pillow>=10.0.0,<13.0.0 # Pure-Python media metadata parser fallback (audio/video) when ffprobe is missing -hachoir>=3.3.0 +hachoir>=3.3.0,<4.0.0 # Safe file deletion (recycle bin support) -send2trash>=1.8.0 +send2trash>=1.8.0,<2.0.0 # File system monitoring for auto-reindexing (optional, enable with MJR_ENABLE_WATCHER=1) -watchdog>=3.0.0 +watchdog>=3.0.0,<7.0.0 # Windows-specific: Windows Property System metadata support # Only required on Windows; gracefully skipped on other platforms -pywin32>=300; sys_platform == 'win32' +pywin32>=300,<311; sys_platform == 'win32' # Note: tkinter is required for native folder browser functionality # tkinter is included in standard Python distribution but may be missing in minimal installs -# Testing helpers -pytest-asyncio>=0.22.0 - -# Static analysis -pyflakes>=3.4.0 +# Dev/test tools are intentionally kept out of runtime requirements. diff --git a/tests/compat/test_import_smoke.py b/tests/compat/test_import_smoke.py index 050ae3e8..3db0335f 100644 --- a/tests/compat/test_import_smoke.py +++ b/tests/compat/test_import_smoke.py @@ -2,7 +2,7 @@ import importlib.util from pathlib import Path -def test_import_exposes_init_prompt_server(): +def test_import_exposes_expected_public_symbols(): module_path = Path(__file__).resolve().parents[2] / "__init__.py" spec = importlib.util.spec_from_file_location("mjr_am_ext", module_path) module = importlib.util.module_from_spec(spec) @@ -10,3 +10,9 @@ def test_import_exposes_init_prompt_server(): spec.loader.exec_module(module) assert hasattr(module, "init_prompt_server") assert callable(module.init_prompt_server) + assert hasattr(module, "NODE_CLASS_MAPPINGS") + assert isinstance(module.NODE_CLASS_MAPPINGS, dict) + assert hasattr(module, "WEB_DIRECTORY") + assert isinstance(module.WEB_DIRECTORY, str) + assert hasattr(module, "__version__") + assert isinstance(module.__version__, str) diff --git a/tests/conftest.py b/tests/conftest.py index 31cd3d50..b3c5cc78 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -19,6 +19,24 @@ async def services(tmp_path): try: yield svc finally: + try: + watcher = svc.get("watcher") + if watcher and hasattr(watcher, "stop"): + await watcher.stop() + except Exception: + pass + try: + index = svc.get("index") + if index and hasattr(index, "stop_enrichment"): + await index.stop_enrichment(clear_queue=True) + except Exception: + pass + try: + sync_worker = svc.get("rating_tags_sync") + if sync_worker and hasattr(sync_worker, "stop"): + sync_worker.stop(timeout=1.0) + except Exception: + pass try: await svc.get("db").aclose() except Exception: diff --git a/tests/core/test_request_json.py b/tests/core/test_request_json.py index d7e628dd..38aeaf3f 100644 --- a/tests/core/test_request_json.py +++ b/tests/core/test_request_json.py @@ -32,6 +32,9 @@ def test_content_type_and_length_errors() -> None: bad_ct = _DummyRequest(headers={"Content-Type": "text/plain"}) assert rq._content_type_error(bad_ct) is not None + missing_ct = _DummyRequest(headers={}) + assert rq._content_type_error(missing_ct) is not None + ok_ct = _DummyRequest(headers={"Content-Type": "application/ld+json"}) assert rq._content_type_error(ok_ct) is None @@ -82,3 +85,14 @@ async def test_read_json_end_to_end() -> None: bad_req = _DummyRequest(headers={"Content-Type": "text/plain"}, chunks=[b"{}"]) out2 = await rq._read_json(bad_req, max_bytes=100) assert not out2.ok + + +@pytest.mark.asyncio +async def test_read_json_rejects_content_length_mismatch_over_limit() -> None: + req = _DummyRequest( + headers={"Content-Type": "application/json", "Content-Length": "2"}, + chunks=[b"{", b'"k":"', b"x" * 2048, b'"}'], + ) + out = await rq._read_json(req, max_bytes=64) + assert out.ok is False + assert out.code == "INVALID_INPUT" diff --git a/tests/core/test_shared_utils.py b/tests/core/test_shared_utils.py index c4d40f11..7c00cfee 100644 --- a/tests/core/test_shared_utils.py +++ b/tests/core/test_shared_utils.py @@ -8,6 +8,8 @@ from enum import Enum import pytest +from mjr_am_shared import errors as errors_mod +from mjr_am_shared import log as log_mod from mjr_am_shared import result as result_mod from mjr_am_shared import time as time_mod @@ -102,3 +104,15 @@ def test_result_unwrap_or_default(): def test_result_unwrap_or_ok(): assert result_mod.Result.Ok(7).unwrap_or(99) == 7 + + +def test_get_logger_does_not_duplicate_correlation_filter() -> None: + logger = log_mod.get_logger("test_shared_utils_logger") + logger = log_mod.get_logger("test_shared_utils_logger") + filters = [f for f in list(logger.filters or []) if isinstance(f, log_mod.CorrelationFilter)] + assert len(filters) == 1 + + +def test_sanitize_error_message_does_not_mask_mime_type() -> None: + msg = errors_mod.sanitize_error_message(ValueError("application/json parse failed"), "bad") + assert "application/json" in msg diff --git a/tests/database/test_migration_concurrency.py b/tests/database/test_migration_concurrency.py new file mode 100644 index 00000000..f23e4317 --- /dev/null +++ b/tests/database/test_migration_concurrency.py @@ -0,0 +1,28 @@ +import asyncio + +import pytest + +from mjr_am_backend.adapters.db.schema import migrate_schema +from mjr_am_backend.adapters.db.sqlite_facade import Sqlite + + +@pytest.mark.asyncio +async def test_migrate_schema_while_queries_are_active(tmp_path) -> None: + db = Sqlite(str(tmp_path / "concurrent_migrate.sqlite")) + try: + await db.aexecute("CREATE TABLE IF NOT EXISTS t_conc (id INTEGER PRIMARY KEY, v TEXT)") + await db.aexecute("INSERT INTO t_conc(v) VALUES (?)", ("x",)) + + async def _query_burst(): + for _ in range(8): + out = await db.aquery("SELECT id, v FROM t_conc") + assert out.ok + await asyncio.sleep(0) + + query_task = asyncio.create_task(_query_burst()) + migrate_out = await migrate_schema(db) + await query_task + + assert migrate_out.ok + finally: + await db.aclose() diff --git a/tests/features/test_assets_impl_routes.py b/tests/features/test_assets_impl_routes.py index bec081fe..d28c4a5d 100644 --- a/tests/features/test_assets_impl_routes.py +++ b/tests/features/test_assets_impl_routes.py @@ -169,7 +169,7 @@ class _DummyDB: return Result.Ok([]) async def aquery_in(self, _sql, _column, values): - return Result.Ok([{"id": int(v), "filepath": f"C:/tmp/{int(v)}.png"} for v in values]) + return Result.Ok([{"id": int(v), "filepath": str(Path("tmp") / f"{int(v)}.png")} for v in values]) async def aexecute(self, sql, params=()): self.execs.append((sql, params)) @@ -782,6 +782,18 @@ async def test_download_asset_rate_and_missing_filepath(monkeypatch): assert resp2.status == 400 +@pytest.mark.asyncio +async def test_download_asset_blocks_path_traversal_query(monkeypatch): + app = web.Application() + app.router.add_get("/mjr/am/asset/download", m.download_asset) + monkeypatch.setattr(m, "_check_rate_limit", lambda *_args, **_kwargs: (True, None)) + monkeypatch.setattr(m, "_is_resolved_path_allowed", lambda _p: False) + + req = make_mocked_request("GET", "/mjr/am/asset/download?filepath=../../etc/passwd", app=app) + resp = await (await app.router.resolve(req)).handler(req) + assert resp.status in {403, 404} + + @pytest.mark.asyncio async def test_delete_asset_forbidden_path(monkeypatch, tmp_path: Path): app = _app() diff --git a/tests/features/test_batch_zip_routes.py b/tests/features/test_batch_zip_routes.py index 10f048cb..e0b64f19 100644 --- a/tests/features/test_batch_zip_routes.py +++ b/tests/features/test_batch_zip_routes.py @@ -36,6 +36,18 @@ def test_batch_zip_sanitize_and_cleanup_helpers(monkeypatch): assert token not in bz._BATCH_CACHE +def test_batch_file_response_sanitizes_content_disposition(tmp_path: Path) -> None: + token = "a" * 40 + zip_path = tmp_path / "payload.zip" + zip_path.write_bytes(b"zip") + entry = {"path": zip_path, "filename": 'bad\r\nname";\x01.zip'} + response = bz._batch_file_response(entry, token) + header = str(response.headers.get("Content-Disposition") or "") + assert "\r" not in header + assert "\n" not in header + assert '"' in header + + @pytest.mark.asyncio async def test_batch_zip_create_invalid_and_get_not_found(monkeypatch): app = _app() @@ -81,7 +93,7 @@ async def test_batch_zip_create_and_fetch_ready(monkeypatch, tmp_path: Path): return Result.Ok({"token": token, "items": [{"filename": "a.png", "subfolder": "", "type": "output"}]}) monkeypatch.setattr(bz, "_read_json", _read_json) - monkeypatch.setattr(bz, "_resolve_item_path", lambda _item: src) + monkeypatch.setattr(bz, "_resolve_item_path", lambda _item: (src, tmp_path)) req = make_mocked_request("POST", "/mjr/am/batch-zip", app=app) match = await app.router.resolve(req) diff --git a/tests/features/test_custom_roots_routes.py b/tests/features/test_custom_roots_routes.py index 19cd7ad6..5d059259 100644 --- a/tests/features/test_custom_roots_routes.py +++ b/tests/features/test_custom_roots_routes.py @@ -38,7 +38,7 @@ async def test_custom_roots_get_and_post_csrf(monkeypatch): @pytest.mark.asyncio -async def test_custom_roots_post_success_with_watcher(monkeypatch): +async def test_custom_roots_post_success_with_watcher(monkeypatch, tmp_path: Path): app = _app() calls = {"added": 0, "scan": 0} @@ -51,7 +51,7 @@ async def test_custom_roots_post_success_with_watcher(monkeypatch): return {"watcher": _Watcher()}, None async def _read_json(_request): - return Result.Ok({"path": "C:/tmp/abc", "label": "abc"}) + return Result.Ok({"path": str(tmp_path / "abc"), "label": "abc"}) async def _scan(*_args, **_kwargs): calls["scan"] += 1 @@ -191,7 +191,7 @@ async def test_browse_folder_csrf_and_rate_limit(monkeypatch): @pytest.mark.asyncio -async def test_custom_roots_remove_success(monkeypatch): +async def test_custom_roots_remove_success(monkeypatch, tmp_path: Path): app = _app() calls = {"removed": 0} @@ -210,7 +210,7 @@ async def test_custom_roots_remove_success(monkeypatch): monkeypatch.setattr(m, "_csrf_error", lambda _request: None) monkeypatch.setattr(m, "_require_write_access", lambda _request: Result.Ok({})) monkeypatch.setattr(m, "_read_json", _read_json) - monkeypatch.setattr(m, "resolve_custom_root", lambda _rid: Result.Ok(Path("C:/tmp/root"))) + monkeypatch.setattr(m, "resolve_custom_root", lambda _rid: Result.Ok(tmp_path / "root")) monkeypatch.setattr(m, "remove_custom_root", lambda _rid: Result.Ok({"removed": True})) monkeypatch.setattr(m, "_require_services", _services) @@ -223,14 +223,14 @@ async def test_custom_roots_remove_success(monkeypatch): @pytest.mark.asyncio -async def test_custom_view_guards(monkeypatch): +async def test_custom_view_guards(monkeypatch, tmp_path: Path): app = _app() req1 = make_mocked_request("GET", "/mjr/am/custom-view", app=app) resp1 = await (await app.router.resolve(req1)).handler(req1) assert _json(resp1).get("code") == "INVALID_INPUT" - monkeypatch.setattr(m, "resolve_custom_root", lambda _rid: Result.Ok(Path("C:/tmp"))) + monkeypatch.setattr(m, "resolve_custom_root", lambda _rid: Result.Ok(tmp_path)) monkeypatch.setattr(m, "_safe_rel_path", lambda _s: None) req2 = make_mocked_request("GET", "/mjr/am/custom-view?root_id=r1&filename=a.png&subfolder=..", app=app) resp2 = await (await app.router.resolve(req2)).handler(req2) @@ -371,7 +371,7 @@ async def test_browser_folder_op_move_and_invalid(monkeypatch, tmp_path: Path): assert _json(resp2).get("code") == "INVALID_INPUT" @pytest.mark.asyncio -async def test_browse_folder_headless_and_success_and_cancel(monkeypatch): +async def test_browse_folder_headless_and_success_and_cancel(monkeypatch, tmp_path: Path): app = _app() monkeypatch.setattr(m, "_csrf_error", lambda _request: None) monkeypatch.setattr(m, "_check_rate_limit", lambda *_args, **_kwargs: (True, None)) @@ -415,7 +415,7 @@ async def test_browse_folder_headless_and_success_and_cancel(monkeypatch): @staticmethod def askdirectory(title=None): _ = title - return "C:/tmp" + return str(tmp_path) monkeypatch.setattr(m, "filedialog", _FD2) req3 = make_mocked_request("POST", "/mjr/sys/browse-folder", app=app) diff --git a/tests/features/test_db_maintenance_more.py b/tests/features/test_db_maintenance_more.py index 266c36dc..ae77d59e 100644 --- a/tests/features/test_db_maintenance_more.py +++ b/tests/features/test_db_maintenance_more.py @@ -78,10 +78,10 @@ async def test_db_backup_restore_not_found_and_save_service_unavailable(monkeypa monkeypatch.setattr(m, "_require_services", _require_services_ok) req_restore = make_mocked_request("POST", "/mjr/am/db/backup-restore", app=app) - async def _json(): - return {"use_latest": True} + async def _read_json(_request): + return Result.Ok({"use_latest": True}) - req_restore.json = _json + monkeypatch.setattr(m, "_read_json", _read_json) match_restore = await app.router.resolve(req_restore) resp_restore = await match_restore.handler(req_restore) body_restore = json.loads(resp_restore.text) @@ -101,10 +101,10 @@ async def test_db_backup_restore_requested_name_not_found_and_missing_db(monkeyp monkeypatch.setattr(m, "_require_services", _require_services_missing_db) req = make_mocked_request("POST", "/mjr/am/db/backup-restore", app=app) - async def _json(): - return {"name": "missing.sqlite"} + async def _read_json(_request): + return Result.Ok({"name": "missing.sqlite"}) - req.json = _json + monkeypatch.setattr(m, "_read_json", _read_json) match = await app.router.resolve(req) resp = await match.handler(req) body = json.loads(resp.text) @@ -130,10 +130,10 @@ async def test_db_backup_restore_reset_fail_and_replace_fail(monkeypatch, tmp_pa monkeypatch.setattr(m, "_require_services", _require_services_reset_fail) req1 = make_mocked_request("POST", "/mjr/am/db/backup-restore", app=app) - async def _json_latest(): - return {"use_latest": True} + async def _read_json(_request): + return Result.Ok({"use_latest": True}) - req1.json = _json_latest + monkeypatch.setattr(m, "_read_json", _read_json) match1 = await app.router.resolve(req1) resp1 = await match1.handler(req1) body1 = json.loads(resp1.text) @@ -156,7 +156,6 @@ async def test_db_backup_restore_reset_fail_and_replace_fail(monkeypatch, tmp_pa monkeypatch.setattr(m, "_replace_db_from_backup", _replace_fail) req2 = make_mocked_request("POST", "/mjr/am/db/backup-restore", app=app) - req2.json = _json_latest match2 = await app.router.resolve(req2) resp2 = await match2.handler(req2) body2 = json.loads(resp2.text) diff --git a/tests/features/test_db_maintenance_routes.py b/tests/features/test_db_maintenance_routes.py index 0538e068..57501ccd 100644 --- a/tests/features/test_db_maintenance_routes.py +++ b/tests/features/test_db_maintenance_routes.py @@ -282,6 +282,9 @@ async def test_db_backup_restore_success(monkeypatch, tmp_path: Path): monkeypatch.setattr(m, "_require_write_access", lambda _request: Result.Ok({})) monkeypatch.setattr(m, "_require_services", _require_services) monkeypatch.setattr(m, "get_runtime_output_root", lambda: str(tmp_path)) + async def _read_json(_request): + return Result.Ok({"use_latest": True}) + monkeypatch.setattr(m, "_read_json", _read_json) def _create_task(coro): try: @@ -294,11 +297,6 @@ async def test_db_backup_restore_success(monkeypatch, tmp_path: Path): app = _app() req = make_mocked_request("POST", "/mjr/am/db/backup-restore", app=app) - - async def _json(): - return {"use_latest": True} - - req.json = _json match = await app.router.resolve(req) resp = await match.handler(req) payload = json.loads(resp.text) diff --git a/tests/features/test_db_recovery_extra.py b/tests/features/test_db_recovery_extra.py index 09c709a2..891aadf2 100644 --- a/tests/features/test_db_recovery_extra.py +++ b/tests/features/test_db_recovery_extra.py @@ -55,6 +55,22 @@ def test_populate_table_columns_cursor_execute_raises() -> None: assert len(known) == 0 +def test_populate_table_columns_rejects_unsafe_table_identifier() -> None: + calls = {"execute": 0} + + class _Cursor: + def execute(self, _sql: str) -> None: + calls["execute"] += 1 + + def fetchall(self): + return [(0, "id")] + + known: set[str] = set() + dr.populate_table_columns(_Cursor(), "assets;DROP TABLE assets--", "a", known) + assert calls["execute"] == 0 + assert not known + + def test_populate_known_columns_from_schema_nonexistent_db(tmp_path: Path) -> None: """Line 143 — db_path does not exist → early return with empty lowercase map.""" db = tmp_path / "does_not_exist.db" diff --git a/tests/features/test_ffprobe_adapter.py b/tests/features/test_ffprobe_adapter.py new file mode 100644 index 00000000..561a69f6 --- /dev/null +++ b/tests/features/test_ffprobe_adapter.py @@ -0,0 +1,62 @@ +import pytest + +from mjr_am_backend.adapters.tools.ffprobe import FFProbe +from mjr_am_backend.shared import ErrorCode + + +def _new_probe() -> FFProbe: + probe = FFProbe.__new__(FFProbe) + probe.bin = "ffprobe" + probe.timeout = 1.0 + probe._resolved_bin = "ffprobe" + probe._available = True + probe._max_workers = 1 + return probe + + +def test_validate_probe_path_rejects_invalid_inputs() -> None: + probe = _new_probe() + + bad_inputs = ["", " ", "-show_streams", "a\x00b", "a\nb", "a\rb"] + for value in bad_inputs: + out = FFProbe._validate_probe_path(probe, value) + assert out.ok is False + assert out.code == ErrorCode.INVALID_INPUT + + +def test_validate_probe_path_accepts_normal_file_path() -> None: + probe = _new_probe() + out = FFProbe._validate_probe_path(probe, " ./video.mp4 ") + assert out.ok is True + assert out.data == "./video.mp4" + + +def test_read_rejects_invalid_path_before_subprocess(monkeypatch) -> None: + probe = _new_probe() + calls = {"run": 0} + + def _run(_cmd): + calls["run"] += 1 + raise AssertionError("subprocess should not run for invalid paths") + + monkeypatch.setattr(probe, "_run_ffprobe_cmd", _run) + out = probe.read("-bad") + assert out.ok is False + assert out.code == ErrorCode.INVALID_INPUT + assert calls["run"] == 0 + + +@pytest.mark.asyncio +async def test_aread_rejects_invalid_path_before_subprocess(monkeypatch) -> None: + probe = _new_probe() + calls = {"spawn": 0} + + async def _spawn(_cmd): + calls["spawn"] += 1 + raise AssertionError("subprocess should not run for invalid paths") + + monkeypatch.setattr(probe, "_spawn_ffprobe_process", _spawn) + out = await probe.aread("-bad") + assert out.ok is False + assert out.code == ErrorCode.INVALID_INPUT + assert calls["spawn"] == 0 diff --git a/tests/features/test_health_routes.py b/tests/features/test_health_routes.py index 11ee0799..2e633053 100644 --- a/tests/features/test_health_routes.py +++ b/tests/features/test_health_routes.py @@ -129,6 +129,32 @@ async def test_update_output_directory_csrf_block(monkeypatch) -> None: body = json.loads(resp.text) assert body.get("code") == "CSRF" + +@pytest.mark.asyncio +async def test_update_output_directory_real_csrf_and_auth_guards(monkeypatch) -> None: + monkeypatch.setenv("MAJOOR_REQUIRE_AUTH", "1") + monkeypatch.delenv("MAJOOR_API_TOKEN", raising=False) + monkeypatch.delenv("MJR_API_TOKEN", raising=False) + monkeypatch.delenv("MAJOOR_API_TOKEN_HASH", raising=False) + monkeypatch.delenv("MJR_API_TOKEN_HASH", raising=False) + + app = _build_health_app() + + req_missing_csrf = make_mocked_request("POST", "/mjr/am/settings/output-directory", app=app) + resp_missing_csrf = await (await app.router.resolve(req_missing_csrf)).handler(req_missing_csrf) + body_missing_csrf = json.loads(resp_missing_csrf.text) + assert body_missing_csrf.get("code") == "CSRF" + + req_missing_auth = make_mocked_request( + "POST", + "/mjr/am/settings/output-directory", + headers={"X-Requested-With": "XMLHttpRequest"}, + app=app, + ) + resp_missing_auth = await (await app.router.resolve(req_missing_auth)).handler(req_missing_auth) + body_missing_auth = json.loads(resp_missing_auth.text) + assert body_missing_auth.get("code") == "AUTH_REQUIRED" + class _Settings: def __init__(self): self.output = "" @@ -176,7 +202,7 @@ class _Settings: @pytest.mark.asyncio -async def test_health_and_counters_success_timeout_degraded(monkeypatch) -> None: +async def test_health_and_counters_success_timeout_degraded(monkeypatch, tmp_path: Path) -> None: class _Health: async def status(self): return Result.Ok({"up": True}) @@ -189,7 +215,7 @@ async def test_health_and_counters_success_timeout_degraded(monkeypatch) -> None return {"health": _Health(), "index": object()}, None monkeypatch.setattr(health_mod, "_require_services", _svc) - monkeypatch.setattr(health_mod, "resolve_custom_root", lambda _rid: Result.Ok(Path("C:/tmp"))) + monkeypatch.setattr(health_mod, "resolve_custom_root", lambda _rid: Result.Ok(tmp_path)) app = _build_health_app() req1 = make_mocked_request("GET", "/mjr/am/health", app=app) @@ -259,14 +285,14 @@ async def test_health_db_success_and_error(monkeypatch) -> None: @pytest.mark.asyncio -async def test_output_and_probe_settings_routes(monkeypatch) -> None: +async def test_output_and_probe_settings_routes(monkeypatch, tmp_path: Path) -> None: settings = _Settings() async def _svc(): return {"settings": settings}, None async def _read_json(_request): - return Result.Ok({"output_directory": "C:/out", "mode": "ffprobe"}) + return Result.Ok({"output_directory": str(tmp_path), "mode": "ffprobe"}) monkeypatch.setattr(health_mod, "_require_services", _svc) monkeypatch.setattr(health_mod, "_csrf_error", lambda _req: None) @@ -361,6 +387,8 @@ async def test_security_empty_input_and_token_routes(monkeypatch) -> None: monkeypatch.setattr(health_mod, "_require_write_access", lambda _req: Result.Ok({})) monkeypatch.setattr(health_mod, "_has_configured_write_token", lambda: False) monkeypatch.setattr(health_mod, "_read_json", _read_empty) + monkeypatch.setattr(health_mod, "_is_secure_request_transport", lambda _req: True) + monkeypatch.setattr(health_mod, "_bootstrap_enabled", lambda: True) app = _build_health_app() req1 = make_mocked_request("POST", "/mjr/am/settings/security", app=app) @@ -387,6 +415,7 @@ async def test_bootstrap_token_blocked_when_token_already_configured(monkeypatch monkeypatch.setattr(health_mod, "_csrf_error", lambda _req: None) monkeypatch.setattr(health_mod, "_require_write_access", lambda _req: Result.Ok({})) monkeypatch.setattr(health_mod, "_has_configured_write_token", lambda: True) + monkeypatch.setattr(health_mod, "_bootstrap_enabled", lambda: True) app = _build_health_app() req = make_mocked_request("POST", "/mjr/am/settings/security/bootstrap-token", app=app) @@ -394,6 +423,47 @@ async def test_bootstrap_token_blocked_when_token_already_configured(monkeypatch assert json.loads(resp.text).get("code") == "FORBIDDEN" +@pytest.mark.asyncio +async def test_bootstrap_token_disabled_without_env_gate(monkeypatch) -> None: + settings = _Settings() + + async def _svc(): + return {"settings": settings}, None + + monkeypatch.setattr(health_mod, "_require_services", _svc) + monkeypatch.setattr(health_mod, "_csrf_error", lambda _req: None) + monkeypatch.setattr(health_mod, "_require_write_access", lambda _req: Result.Ok({})) + monkeypatch.setattr(health_mod, "_has_configured_write_token", lambda: False) + monkeypatch.setattr(health_mod, "_bootstrap_enabled", lambda: False) + + app = _build_health_app() + req = make_mocked_request("POST", "/mjr/am/settings/security/bootstrap-token", app=app) + resp = await (await app.router.resolve(req)).handler(req) + assert json.loads(resp.text).get("code") == "BOOTSTRAP_DISABLED" + + +@pytest.mark.asyncio +async def test_bootstrap_token_requires_authenticated_comfy_user_when_write_auth_fails(monkeypatch) -> None: + settings = _Settings() + + async def _svc(): + return {"settings": settings}, None + + monkeypatch.setattr(health_mod, "_require_services", _svc) + monkeypatch.setattr(health_mod, "_csrf_error", lambda _req: None) + monkeypatch.setattr(health_mod, "_require_write_access", lambda _req: Result.Err("AUTH_REQUIRED", "missing")) + monkeypatch.setattr(health_mod, "_is_loopback_request", lambda _req: True) + monkeypatch.setattr(health_mod, "_require_authenticated_user", lambda _req: Result.Ok("", auth_mode="disabled")) + monkeypatch.setattr(health_mod, "_has_configured_write_token", lambda: False) + monkeypatch.setattr(health_mod, "_is_secure_request_transport", lambda _req: True) + monkeypatch.setattr(health_mod, "_bootstrap_enabled", lambda: True) + + app = _build_health_app() + req = make_mocked_request("POST", "/mjr/am/settings/security/bootstrap-token", app=app) + resp = await (await app.router.resolve(req)).handler(req) + assert json.loads(resp.text).get("code") == "AUTH_REQUIRED" + + @pytest.mark.asyncio async def test_tools_and_roots(monkeypatch, tmp_path) -> None: async def _svc(): diff --git a/tests/features/test_index_searcher.py b/tests/features/test_index_searcher.py index 85d269d3..3d4a69e5 100644 --- a/tests/features/test_index_searcher.py +++ b/tests/features/test_index_searcher.py @@ -46,7 +46,7 @@ def test_helper_functions_cover_smoke(): assert m._normalize_pagination(999999, 99999999)[0] == m.SEARCH_MAX_LIMIT assert m._escape_like_pattern("a%_\\") assert m._resolve_search_roots(["."]) - where, params = m._build_roots_where_clause(["C:/tmp"]) + where, params = m._build_roots_where_clause(["/tmp"]) assert "LIKE" in where and params assert m._normalize_month_range("1", "2") == (1, 2) assert m._sanitize_histogram_filters({"mtime_start": 1, "x": 2}) == {"x": 2} diff --git a/tests/features/test_index_watcher.py b/tests/features/test_index_watcher.py index f0e0b838..be10b39e 100644 --- a/tests/features/test_index_watcher.py +++ b/tests/features/test_index_watcher.py @@ -119,8 +119,8 @@ def test_apply_flush_limit(monkeypatch): def test_handler_path_support_and_ignore(handler): assert handler._is_supported("x.png") is True assert handler._is_supported("x.txt") is False - assert handler._is_ignored_path("C:/tmp/.git/a.png") is True - assert handler._is_ignored_path("C:/tmp/ok/a.png") is False + assert handler._is_ignored_path("/tmp/.git/a.png") is True + assert handler._is_ignored_path("/tmp/ok/a.png") is False def test_handler_normalize_path_fallback(handler, monkeypatch): @@ -143,11 +143,11 @@ def test_handler_pending_count(handler): def test_handle_file_rejects_and_adds(handler): handler._handle_file("") - handler._handle_file("C:/tmp/.git/a.png") - handler._handle_file("C:/tmp/a.txt") + handler._handle_file("/tmp/.git/a.png") + handler._handle_file("/tmp/a.txt") assert not handler._pending - handler._handle_file("C:/tmp/a.png") + handler._handle_file("/tmp/a.png") assert len(handler._pending) == 1 @@ -209,10 +209,10 @@ async def test_flush_pipeline_filters_backpressure_and_mark(monkeypatch, handler monkeypatch.setattr(w, "MAX_FILE_SIZE", 10_000) monkeypatch.setattr(w.os.path, "getsize", lambda p: 100 if "ok" in p else 0) handler._pending = { - "C:/tmp/ok1.png": 1, - "C:/tmp/ok2.png": 1, - "C:/tmp/ok3.png": 1, - "C:/tmp/no.txt": 1, + "/tmp/ok1.png": 1, + "/tmp/ok2.png": 1, + "/tmp/ok3.png": 1, + "/tmp/no.txt": 1, } flushed = [] diff --git a/tests/features/test_releases_routes.py b/tests/features/test_releases_routes.py new file mode 100644 index 00000000..453403aa --- /dev/null +++ b/tests/features/test_releases_routes.py @@ -0,0 +1,62 @@ +import json +from urllib.parse import urlencode + +import pytest +from aiohttp import web +from aiohttp.test_utils import make_mocked_request + +from mjr_am_backend.routes.handlers import releases as rel + + +def _app() -> web.Application: + app = web.Application() + routes = web.RouteTableDef() + rel.register_releases_routes(routes) + app.add_routes(routes) + return app + + +@pytest.mark.asyncio +async def test_releases_rejects_invalid_owner_repo_segments() -> None: + app = _app() + cases = [ + {"owner": "..", "repo": "repo"}, + {"owner": "owner", "repo": "bad..repo"}, + {"owner": "owner name", "repo": "repo"}, + {"owner": "owner", "repo": "bad@repo"}, + ] + for case in cases: + query = urlencode(case) + req = make_mocked_request("GET", f"/mjr/am/releases?{query}", app=app) + match = await app.router.resolve(req) + resp = await match.handler(req) + payload = json.loads(resp.text) + assert payload.get("code") == "INVALID_INPUT" + + +@pytest.mark.asyncio +async def test_releases_returns_refs_for_valid_owner_repo(monkeypatch) -> None: + async def _fake_fetch(_session, url, _headers): + if "/tags?" in url: + return [{"name": "v1.0.0"}, {"name": "v1.0.1"}] + if "/branches?" in url: + return [{"name": "main"}, {"name": "dev"}] + return [] + + monkeypatch.setattr(rel, "_fetch_github_json", _fake_fetch) + + app = _app() + req = make_mocked_request( + "GET", + "/mjr/am/releases?owner=MajoorWaldi&repo=ComfyUI-Majoor-AssetsManager&per_page=50", + app=app, + ) + match = await app.router.resolve(req) + resp = await match.handler(req) + payload = json.loads(resp.text) + assert payload.get("ok") is True + data = payload.get("data") or {} + assert data.get("owner") == "MajoorWaldi" + assert data.get("repo") == "ComfyUI-Majoor-AssetsManager" + assert data.get("tags") == ["v1.0.0", "v1.0.1"] + assert data.get("branches") == ["main", "dev"] diff --git a/tests/features/test_scan_throttle.py b/tests/features/test_scan_throttle.py index cc836bdb..e51608e2 100644 --- a/tests/features/test_scan_throttle.py +++ b/tests/features/test_scan_throttle.py @@ -10,9 +10,7 @@ from mjr_am_shared import scan_throttle as th def _reset_state(): """Clear module-level dicts between tests.""" - with th._LOCK: - th._MANUAL_SCAN_TIMES.clear() - th._RECENT_SCAN_TIMES.clear() + th._reset_scan_throttle_state_for_tests() @pytest.fixture(autouse=True) @@ -94,3 +92,11 @@ def test_cleanup_locked_removes_old(): th._cleanup_locked(now) assert "old_key" not in th._MANUAL_SCAN_TIMES assert "old_key" not in th._RECENT_SCAN_TIMES + + +def test_reset_helper_clears_state(): + th.mark_directory_indexed("/x") + th.mark_directory_scanned("/x") + th._reset_scan_throttle_state_for_tests() + assert not th._MANUAL_SCAN_TIMES + assert not th._RECENT_SCAN_TIMES diff --git a/tests/features/test_scan_upload_routes.py b/tests/features/test_scan_upload_routes.py new file mode 100644 index 00000000..af5c3d80 --- /dev/null +++ b/tests/features/test_scan_upload_routes.py @@ -0,0 +1,37 @@ +import json + +import pytest +from aiohttp import web +from aiohttp.test_utils import make_mocked_request + +from mjr_am_backend.routes.handlers import scan_upload as upload_mod +from mjr_am_backend.shared import Result + + +def _app(deps: dict) -> web.Application: + app = web.Application() + routes = web.RouteTableDef() + upload_mod.register_upload_routes(routes, deps=deps) + app.add_routes(routes) + return app + + +@pytest.mark.asyncio +async def test_upload_input_rejects_oversized_content_length(monkeypatch) -> None: + monkeypatch.setattr(upload_mod, "_MAX_UPLOAD_SIZE", 1024) + + deps = { + "_csrf_error": lambda _req: None, + "_require_write_access": lambda _req: Result.Ok({}), + } + app = _app(deps) + req = make_mocked_request( + "POST", + "/mjr/am/upload_input", + headers={"Content-Length": "2048"}, + app=app, + ) + match = await app.router.resolve(req) + resp = await match.handler(req) + payload = json.loads(resp.text) + assert payload.get("code") == "FILE_TOO_LARGE" diff --git a/tests/features/test_settings_backend.py b/tests/features/test_settings_backend.py index 493d4036..98a8966c 100644 --- a/tests/features/test_settings_backend.py +++ b/tests/features/test_settings_backend.py @@ -46,7 +46,7 @@ class _DB: @pytest.fixture(autouse=True) -def _clean_token_env(): +def _clean_token_env(monkeypatch): keys = ( "MAJOOR_API_TOKEN", "MJR_API_TOKEN", @@ -55,14 +55,14 @@ def _clean_token_env(): "MAJOOR_API_TOKEN_PEPPER", ) for k in keys: - os.environ.pop(k, None) + monkeypatch.delenv(k, raising=False) yield try: sec._reset_security_state_for_tests() except Exception: pass for k in keys: - os.environ.pop(k, None) + monkeypatch.delenv(k, raising=False) @pytest.mark.asyncio @@ -105,14 +105,15 @@ async def test_get_or_create_api_token_from_env(monkeypatch): @pytest.mark.asyncio -async def test_get_or_create_api_token_from_hash_regenerates(monkeypatch): +async def test_get_or_create_api_token_from_hash_loads_hash_without_regeneration(monkeypatch): db = _DB() s = AppSettings(db) db.store[_SECURITY_API_TOKEN_HASH_KEY] = s._hash_api_token("old") monkeypatch.delenv("MAJOOR_API_TOKEN", raising=False) t = await s._get_or_create_api_token_locked() - assert t - assert t != "old" + assert t == "" + assert os.environ.get("MAJOOR_API_TOKEN") is None + assert os.environ.get("MAJOOR_API_TOKEN_HASH") == db.store[_SECURITY_API_TOKEN_HASH_KEY] @pytest.mark.asyncio diff --git a/tests/regressions/test_db_reset_async_safety.py b/tests/regressions/test_db_reset_async_safety.py index b7243b3e..32ae0ee7 100644 --- a/tests/regressions/test_db_reset_async_safety.py +++ b/tests/regressions/test_db_reset_async_safety.py @@ -1,24 +1,94 @@ -from tests.repo_root import REPO_ROOT +import types +import threading + +import pytest + +from mjr_am_backend.adapters.db import sqlite_facade as sf +from mjr_am_backend.shared import Result -def test_areset_does_not_call_sync_init_from_async_flow() -> None: - p = REPO_ROOT / "mjr_am_backend" / "adapters" / "db" / "sqlite_facade.py" - s = p.read_text(encoding="utf-8", errors="replace") - marker = "async def areset(self) -> Result[bool]:" - i = s.find(marker) - assert i >= 0 - body = s[i : i + 3500] - assert "await self._ensure_initialized_async(" in body - assert "self._init_db()" not in body +def _make_stub_sqlite() -> sf.Sqlite: + db = sf.Sqlite.__new__(sf.Sqlite) + db._lock = threading.Lock() + db._initialized = True + db._resetting = False + return db -def test_areset_unlocks_after_reinit_success() -> None: - p = REPO_ROOT / "mjr_am_backend" / "adapters" / "db" / "sqlite_facade.py" - s = p.read_text(encoding="utf-8", errors="replace") - marker = "async def areset(self) -> Result[bool]:" - i = s.find(marker) - assert i >= 0 - body = s[i : i + 4500] - assert "self._resetting = True" in body - assert "Release reset guard before schema helpers" in body - assert "self._resetting = False" in body +@pytest.mark.asyncio +async def test_areset_uses_async_reinit_and_not_sync_init(monkeypatch) -> None: + db = _make_stub_sqlite() + calls: list[str] = [] + + async def _drain(self): + calls.append("drain") + + async def _delete(self, deleted_files, renamed_files): + deleted_files.append("x.sqlite") + _ = renamed_files + calls.append("delete") + return None + + async def _ensure(self, *, allow_during_reset: bool = False): + calls.append(f"ensure:{allow_during_reset}") + + def _init_db(self): + calls.append("init_db") + + import mjr_am_backend.adapters.db.schema as schema_mod + + async def _tables(_db): + return Result.Ok(True) + + async def _indexes(_db): + return Result.Ok(True) + + db._drain_for_reset = types.MethodType(_drain, db) + db._delete_reset_files = types.MethodType(_delete, db) + db._ensure_initialized_async = types.MethodType(_ensure, db) + db._init_db = types.MethodType(_init_db, db) + monkeypatch.setattr(schema_mod, "ensure_tables_exist", _tables) + monkeypatch.setattr(schema_mod, "ensure_indexes_and_triggers", _indexes) + + out = await sf.Sqlite.areset(db) + assert out.ok is True + assert "drain" in calls + assert "delete" in calls + assert "ensure:True" in calls + assert "init_db" not in calls + assert db._resetting is False + + +@pytest.mark.asyncio +async def test_areset_unlocks_when_schema_step_fails(monkeypatch) -> None: + db = _make_stub_sqlite() + + async def _drain(self): + return None + + async def _delete(self, deleted_files, renamed_files): + _ = (deleted_files, renamed_files) + return None + + async def _ensure(self, *, allow_during_reset: bool = False): + _ = allow_during_reset + return None + + import mjr_am_backend.adapters.db.schema as schema_mod + + async def _tables(_db): + return Result.Err("DB_ERROR", "schema failed") + + async def _indexes(_db): + return Result.Ok(True) + + db._drain_for_reset = types.MethodType(_drain, db) + db._delete_reset_files = types.MethodType(_delete, db) + db._ensure_initialized_async = types.MethodType(_ensure, db) + monkeypatch.setattr(schema_mod, "ensure_tables_exist", _tables) + monkeypatch.setattr(schema_mod, "ensure_indexes_and_triggers", _indexes) + + out = await sf.Sqlite.areset(db) + assert out.ok is False + assert out.code == "DB_ERROR" + assert db._resetting is False diff --git a/tests/regressions/test_route_security_guards.py b/tests/regressions/test_route_security_guards.py index bf590a4f..a12d2e21 100644 --- a/tests/regressions/test_route_security_guards.py +++ b/tests/regressions/test_route_security_guards.py @@ -1,68 +1,126 @@ -from tests.repo_root import REPO_ROOT +import json +from pathlib import Path + +import pytest +from aiohttp import web +from aiohttp.test_utils import make_mocked_request + +from mjr_am_backend.routes.core import security as sec +from mjr_am_backend.routes.handlers import collections as collections_mod +from mjr_am_backend.routes.handlers import custom_roots as custom_roots_mod +from mjr_am_backend.routes.handlers import health as health_mod +from mjr_am_backend.shared import Result -def _read(path: str) -> str: - p = REPO_ROOT / path - return p.read_text(encoding="utf-8", errors="replace") +def _app(register_fn): + app = web.Application() + routes = web.RouteTableDef() + register_fn(routes) + app.add_routes(routes) + return app -def test_custom_view_filepath_mode_is_allowlisted() -> None: - s = _read("mjr_am_backend/routes/handlers/custom_roots.py") - assert "_normalize_path(filepath)" in s - assert "_is_path_allowed(candidate, must_exist=True)" in s - assert "browser_mode" in s - assert "_is_loopback_request(request)" in s - assert "or browser_mode" not in s +def _json(resp): + return json.loads(resp.text) -def test_folder_info_filepath_mode_is_allowlisted() -> None: - s = _read("mjr_am_backend/routes/handlers/custom_roots.py") - assert "_is_path_allowed(normalized, must_exist=True)" in s - assert "browser_mode" in s - assert "_is_loopback_request(request)" in s - assert "or browser_mode" not in s - assert 'Result.Err("FORBIDDEN", "Path is not within allowed roots")' in s +@pytest.mark.asyncio +async def test_custom_view_filepath_mode_rejects_outside_roots(monkeypatch, tmp_path: Path) -> None: + app = _app(custom_roots_mod.register_custom_roots_routes) + f = tmp_path / "x.png" + f.write_bytes(b"x") + + monkeypatch.setattr(custom_roots_mod, "_normalize_path", lambda _v: f) + monkeypatch.setattr(custom_roots_mod, "_is_path_allowed", lambda *_a, **_k: False) + monkeypatch.setattr(custom_roots_mod, "_is_path_allowed_custom", lambda *_a, **_k: False) + + req = make_mocked_request("GET", "/mjr/am/custom-view?filepath=x&browser_mode=0", app=app) + resp = await (await app.router.resolve(req)).handler(req) + assert _json(resp).get("code") == "FORBIDDEN" -def test_collections_mutations_have_csrf_and_write_access_guards() -> None: - s = _read("mjr_am_backend/routes/handlers/collections.py") - assert s.count("csrf = _csrf_error(request)") >= 4 - assert s.count("auth = _require_write_access(request)") >= 4 +@pytest.mark.asyncio +async def test_folder_info_filepath_mode_rejects_outside_roots(monkeypatch, tmp_path: Path) -> None: + app = _app(custom_roots_mod.register_custom_roots_routes) + d = tmp_path / "dir" + d.mkdir() + + monkeypatch.setattr(custom_roots_mod, "_normalize_path", lambda _v: d) + monkeypatch.setattr(custom_roots_mod, "_is_path_allowed", lambda *_a, **_k: False) + monkeypatch.setattr(custom_roots_mod, "_is_path_allowed_custom", lambda *_a, **_k: False) + + req = make_mocked_request("GET", "/mjr/am/folder-info?filepath=x", app=app) + resp = await (await app.router.resolve(req)).handler(req) + assert _json(resp).get("code") == "FORBIDDEN" -def test_settings_mutations_require_write_access() -> None: - s = _read("mjr_am_backend/routes/handlers/health.py") - assert "@routes.post(\"/mjr/am/settings/probe-backend\")" in s - assert "@routes.post(\"/mjr/am/settings/metadata-fallback\")" in s - assert s.count("auth = _require_write_access(request)") >= 4 +@pytest.mark.asyncio +async def test_collections_mutations_require_csrf_and_write_access(monkeypatch) -> None: + app = _app(collections_mod.register_collections_routes) + + monkeypatch.setattr(collections_mod, "_csrf_error", lambda _request: "csrf blocked") + req1 = make_mocked_request("POST", "/mjr/am/collections", app=app) + resp1 = await (await app.router.resolve(req1)).handler(req1) + assert _json(resp1).get("code") == "CSRF" + + monkeypatch.setattr(collections_mod, "_csrf_error", lambda _request: None) + monkeypatch.setattr(collections_mod, "_require_write_access", lambda _request: Result.Err("AUTH_REQUIRED", "auth required")) + req2 = make_mocked_request("POST", "/mjr/am/collections", app=app) + resp2 = await (await app.router.resolve(req2)).handler(req2) + assert _json(resp2).get("code") == "AUTH_REQUIRED" -def test_browser_folder_ops_require_csrf() -> None: - s = _read("mjr_am_backend/routes/handlers/custom_roots.py") - assert "@routes.post(\"/mjr/am/browser/folder-op\")" in s - assert "csrf = _csrf_error(request)" in s +@pytest.mark.asyncio +async def test_settings_mutation_requires_write_access(monkeypatch) -> None: + class _Settings: + async def set_probe_backend(self, mode): + return Result.Ok(mode) + + async def _require_services(): + return {"settings": _Settings()}, None + + async def _read_json(_request): + return Result.Ok({"mode": "auto"}) + + app = _app(health_mod.register_health_routes) + monkeypatch.setattr(health_mod, "_csrf_error", lambda _request: None) + monkeypatch.setattr(health_mod, "_require_write_access", lambda _request: Result.Err("AUTH_REQUIRED", "auth required")) + monkeypatch.setattr(health_mod, "_require_services", _require_services) + monkeypatch.setattr(health_mod, "_read_json", _read_json) + + req = make_mocked_request("POST", "/mjr/am/settings/probe-backend", app=app) + resp = await (await app.router.resolve(req)).handler(req) + assert _json(resp).get("code") == "AUTH_REQUIRED" -def test_browse_folder_requires_csrf() -> None: - s = _read("mjr_am_backend/routes/handlers/custom_roots.py") - assert "@routes.post(\"/mjr/sys/browse-folder\")" in s - assert "csrf = _csrf_error(request)" in s +@pytest.mark.asyncio +async def test_browser_routes_require_csrf(monkeypatch) -> None: + app = _app(custom_roots_mod.register_custom_roots_routes) + monkeypatch.setattr(custom_roots_mod, "_csrf_error", lambda _request: "csrf blocked") + + req1 = make_mocked_request("POST", "/mjr/sys/browse-folder", app=app) + resp1 = await (await app.router.resolve(req1)).handler(req1) + assert _json(resp1).get("code") == "CSRF" + + req2 = make_mocked_request("POST", "/mjr/am/browser/folder-op", app=app) + resp2 = await (await app.router.resolve(req2)).handler(req2) + assert _json(resp2).get("code") == "CSRF" -def test_browser_folder_ops_are_root_confined() -> None: - s = _read("mjr_am_backend/routes/handlers/custom_roots.py") - assert "_is_path_allowed(source, must_exist=True)" in s - assert "_is_path_allowed_custom(source)" in s - assert "_is_path_allowed(dest_dir, must_exist=True)" in s - assert "_is_path_allowed_custom(dest_dir)" in s +def test_rate_limit_overflow_keeps_per_client_state(monkeypatch) -> None: + class _Req: + def __init__(self, cid: str): + self.headers = {"X-Test-Client": cid} + sec._reset_security_state_for_tests() + monkeypatch.setattr(sec, "_MAX_RATE_LIMIT_CLIENTS", 2) + monkeypatch.setattr(sec, "_get_client_identifier", lambda req: str(req.headers.get("X-Test-Client") or "")) -def test_browser_folder_delete_is_not_recursive_by_default() -> None: - s = _read("mjr_am_backend/routes/handlers/custom_roots.py") - assert 'recursive = bool(body.get("recursive", False))' in s + assert sec._check_rate_limit(_Req("a"), "ep", max_requests=1, window_seconds=60)[0] is True + assert sec._check_rate_limit(_Req("b"), "ep", max_requests=1, window_seconds=60)[0] is True + assert sec._check_rate_limit(_Req("c"), "ep", max_requests=1, window_seconds=60)[0] is True - -def test_rate_limit_avoids_shared_overflow_bucket() -> None: - s = _read("mjr_am_backend/routes/core/security.py") - assert "_RATE_LIMIT_OVERFLOW_CLIENT_ID" not in s - assert "_rate_limit_state.popitem(last=False)" in s + # "b" should still have its own tracked state and be rate-limited on second request. + assert sec._check_rate_limit(_Req("b"), "ep", max_requests=1, window_seconds=60)[0] is False + # "a" was evicted as oldest, so it should be treated as new. + assert sec._check_rate_limit(_Req("a"), "ep", max_requests=1, window_seconds=60)[0] is True diff --git a/tests/routes/test_assets_path_guard.py b/tests/routes/test_assets_path_guard.py index 5db354dc..023de88b 100644 --- a/tests/routes/test_assets_path_guard.py +++ b/tests/routes/test_assets_path_guard.py @@ -57,3 +57,15 @@ def test_resolve_download_path_and_build_response(monkeypatch, tmp_path: Path): resp = g.build_download_response(f, preview=True) assert resp.headers.get("X-Content-Type-Options") == "nosniff" + + +def test_resolve_download_path_rejects_when_final_open_detects_symlink_swap(monkeypatch, tmp_path: Path): + f = tmp_path / "a.png" + f.write_bytes(b"x") + + monkeypatch.setattr(g, "_normalize_path", lambda _fp: f) + monkeypatch.setattr(g, "is_resolved_path_allowed", lambda _p: True) + monkeypatch.setattr(g, "_validate_no_symlink_open", lambda _p: False) + + out = g.resolve_download_path(str(f)) + assert getattr(out, "status", None) == 403 diff --git a/tests/security/test_security_core_helpers.py b/tests/security/test_security_core_helpers.py index 1da790e4..61f778e2 100644 --- a/tests/security/test_security_core_helpers.py +++ b/tests/security/test_security_core_helpers.py @@ -4,6 +4,8 @@ import pytest from mjr_am_backend.routes.core import security as sec +TEST_TOKEN = "unit-test-token" + class _DummyTransport: def __init__(self, peername): @@ -32,14 +34,17 @@ def test_env_truthy_and_token_extractors(monkeypatch) -> None: assert sec._extract_bearer_token({"Authorization": "Bearer abc"}) == "abc" assert sec._extract_write_token_from_headers({"X-MJR-Token": "x"}) == "x" + assert sec._extract_write_token_from_headers({"Cookie": "mjr_write_token=cookie-token"}) == "cookie-token" def test_hash_and_write_token_hash(monkeypatch) -> None: - monkeypatch.setenv("MAJOOR_API_TOKEN", "secret") + monkeypatch.setenv("MAJOOR_API_TOKEN", TEST_TOKEN) monkeypatch.delenv("MAJOOR_API_TOKEN_HASH", raising=False) + monkeypatch.delenv("MJR_API_TOKEN_HASH", raising=False) + monkeypatch.delenv("MAJOOR_API_TOKEN_PEPPER", raising=False) h = sec._get_write_token_hash() assert h - assert h == sec._hash_token("secret") + assert h == sec._hash_token(TEST_TOKEN) monkeypatch.setenv("MAJOOR_API_TOKEN_HASH", "deadbeef") assert sec._get_write_token_hash() == "deadbeef" @@ -141,6 +146,28 @@ def test_client_identifier_and_rate_limit(monkeypatch) -> None: assert isinstance(retry, int) +def test_check_write_access_uses_constant_time_compare(monkeypatch) -> None: + import hmac + + token = TEST_TOKEN + token_hash = sec._hash_token(token) + monkeypatch.setenv("MAJOOR_API_TOKEN_HASH", token_hash) + monkeypatch.delenv("MAJOOR_API_TOKEN", raising=False) + + calls = {"count": 0} + real_compare = hmac.compare_digest + + def _compare(a, b): + calls["count"] += 1 + return real_compare(a, b) + + monkeypatch.setattr(hmac, "compare_digest", _compare) + + out = sec._check_write_access(peer_ip="127.0.0.1", headers={"X-MJR-Token": token}) + assert out.ok + assert calls["count"] >= 1 + + def test_csrf_and_origin_checks(monkeypatch) -> None: monkeypatch.setattr(sec, "_is_trusted_proxy", lambda _ip: False) diff --git a/tests/security/test_write_access_guard.py b/tests/security/test_write_access_guard.py index b6c804a5..7dff2bd9 100644 --- a/tests/security/test_write_access_guard.py +++ b/tests/security/test_write_access_guard.py @@ -1,6 +1,8 @@ import pytest from mjr_am_backend.routes.core.security import _check_write_access +TEST_TOKEN = "unit-test-token" + def _clear_auth_env(monkeypatch): for key in ( @@ -39,24 +41,43 @@ async def test_write_blocked_for_forwarded_remote_when_no_token_by_default(monke @pytest.mark.asyncio -async def test_write_requires_token_when_configured(monkeypatch): +async def test_write_allows_loopback_even_when_token_configured(monkeypatch): + # Loopback is trusted by default regardless of token configuration. + # A token is validated if provided, but never required from loopback unless + # MAJOOR_REQUIRE_AUTH=1 is explicitly set. _clear_auth_env(monkeypatch) - monkeypatch.setenv("MAJOOR_API_TOKEN", "secret") + monkeypatch.setenv("MAJOOR_API_TOKEN", TEST_TOKEN) + + res = _check_write_access(peer_ip="127.0.0.1", headers={}) + assert res.ok, res.error + assert (res.meta or {}).get("auth") == "loopback" + + res2 = _check_write_access(peer_ip="127.0.0.1", headers={"X-MJR-Token": TEST_TOKEN}) + assert res2.ok, res2.error + assert (res2.meta or {}).get("auth") == "token" + + +@pytest.mark.asyncio +async def test_write_requires_token_on_loopback_when_require_auth_set(monkeypatch): + # MAJOOR_REQUIRE_AUTH=1 forces token auth even for loopback clients. + _clear_auth_env(monkeypatch) + monkeypatch.setenv("MAJOOR_API_TOKEN", TEST_TOKEN) + monkeypatch.setenv("MAJOOR_REQUIRE_AUTH", "1") res = _check_write_access(peer_ip="127.0.0.1", headers={}) assert not res.ok assert res.code == "AUTH_REQUIRED" - res2 = _check_write_access(peer_ip="127.0.0.1", headers={"X-MJR-Token": "secret"}) + res2 = _check_write_access(peer_ip="127.0.0.1", headers={"X-MJR-Token": TEST_TOKEN}) assert res2.ok, res2.error @pytest.mark.asyncio async def test_write_token_accepts_bearer_header(monkeypatch): _clear_auth_env(monkeypatch) - monkeypatch.setenv("MAJOOR_API_TOKEN", "secret") + monkeypatch.setenv("MAJOOR_API_TOKEN", TEST_TOKEN) - res = _check_write_access(peer_ip="127.0.0.1", headers={"Authorization": "Bearer secret"}) + res = _check_write_access(peer_ip="127.0.0.1", headers={"Authorization": f"Bearer {TEST_TOKEN}"}) assert res.ok, res.error @@ -65,7 +86,7 @@ async def test_write_accepts_hashed_token_env(monkeypatch): _clear_auth_env(monkeypatch) import hashlib - token = "secret" + token = TEST_TOKEN token_hash = hashlib.sha256(f"\0{token}".encode()).hexdigest() monkeypatch.setenv("MAJOOR_API_TOKEN_HASH", token_hash) @@ -91,3 +112,19 @@ async def test_require_auth_without_token_blocks(monkeypatch): res = _check_write_access(peer_ip="127.0.0.1", headers={}) assert not res.ok assert res.code == "AUTH_REQUIRED" + + +@pytest.mark.asyncio +async def test_peer_ip_edge_cases_do_not_crash(monkeypatch): + _clear_auth_env(monkeypatch) + + loopback_v6 = _check_write_access(peer_ip="::1", headers={}) + assert loopback_v6.ok + + empty_peer = _check_write_access(peer_ip="", headers={}) + assert not empty_peer.ok + assert empty_peer.code == "FORBIDDEN" + + malformed_peer = _check_write_access(peer_ip="not-an-ip", headers={}) + assert not malformed_peer.ok + assert malformed_peer.code == "FORBIDDEN" diff --git a/{console.error(e) b/{console.error(e) new file mode 100644 index 00000000..e69de29b