Commit 1dbcdcd7 and the comment-cleanup pass 8205022f reduced this to "All
database reads and writes, including the legacy import, run under the lock",
dropping the part that did the work: upstream master locks after migrating and
justifies it with "Alembic uses its own connection, so we must wait until it's
done before locking -- otherwise our own lock blocks the migration". That is
false, the lock is on a separate <db>.lock file, and the surviving sentence
said nothing to stop a contributor "fixing" the ordering back.
Restored and adapted rather than pasted: the legacy copy and the db_exists
probe now happen inside the lock, which the original text predates, so both
are named in the list of things the ordering makes mutually exclusive.
scanner.py reached past the package's own re-export surface to import
_is_live_path_conflict directly out of records.py. The underscore said
module-private while the import said otherwise, and records.py deliberately
publishes its public names through app.assets.database.queries -- which the
same import block three lines above was already using.
The use is correct and unchanged; only the name and the route change. Renamed
to is_live_path_conflict, listed in the package __init__ import and __all__
alongside its siblings, and scanner.py now takes it from the package like
everything else it imports from there.
get_name_and_tags_from_asset_path raises ValueError by contract when a path
stops resolving to a configured root, and it sat outside the guard, as did
compute_loader_path and mimetypes.guess_type. An escape skipped the
_WATCH_LIST[:] = remaining write at the end, so drained entries stayed on the
list and were re-attempted every tick while entries past the fault never
reached the increment _WATCH_SCAN_RETRIES needs to retire them. The list
wedged permanently.
Spec construction is now inside a guard that drops just the offending entry,
and the list write moved into a finally so no future escape can skip it. The
loop walks an iterator rather than the list, so the finally can put back the
entries it never reached instead of discarding them.
New event name rather than reusing one: scanner.watch_seed_failed is emitted
only when seed_asset_specs returns an error, and widening it to also mean
"never got as far as seeding" would make it lie -- a consumer treating it as a
database-health signal would get false positives from what is really a path
layout problem. scanner.watch_spec_failed is registered in ALLOWED_EVENTS and
in the static call-site manifest.
Every other per-file emit on this scan path is gated -- mark_emitted(
"stat_failed:enrich"), "hash_discarded_modified", "hash_failed",
"enrich_failed" -- but scanner.invalid_mtime fired per file, so a restored
archive or a FAT volume of pre-epoch mtimes put one structured event per file
into the stream the closed vocabulary exists to keep parseable.
Counted and emitted once, carrying the count. seed_asset_specs receives no
_ScanProgress object and neither does insert_asset_specs above it, so routing
this through mark_emitted would mean changing both signatures plus the seeder
call site; the count form needs neither and the event is now strictly more
informative than N identical fieldless lines. The per-file logging.warning is
unchanged, and the emit stays inside seed_asset_specs so the static call-site
manifest still matches.
test_seed_skips_negative_fresh_mtime_with_warning_and_telemetry now pins the
full list of invalid_mtime lines to exactly ["... count=1"] instead of
asserting one such line exists -- a strictly stronger assertion, and the only
change the new field required.
Multipart parsing writes the uploaded bytes to a temporary file before it
validates the remaining form fields, so a request rejected after its file
part had already been read left the temp file and its uuid directory on
disk.
Routing those removals through delete_temp_file_if_exists also changes the
success path. The previous helper returned early when the temp file was
already gone, so it never reached the parent rmdir; the shared helper
attempts the rmdir unconditionally. Moving the upload to its destination
leaves the temp path absent, so a successful upload now also discards its
empty uuid directory, closing a pre-existing leak.
The cursor holds at the last row a batch actually attempted, so a pause ends a batch early and the rows behind it are selected again when the scan resumes. Only the attempt is capped at once per pass.
* feat(assets): structured event log lines for the assets system
* test(assets): AST check that tagged event lines stay inside the closed vocabulary
* feat(assets): structured event logging for the seeder lifecycle
* feat(assets): structured event logging for scanner and ingest failure paths
* feat(assets): structured event logging for API request failures
* fix(seeder): finalize checkpoint no longer parks completed scans
* fix(scanner): enrich_failed counts exceptions, not benign races
* fix(seeder): idle reset survives emit/assert failures
* refactor(assets): private emit-once bookkeeping helper; clean Progress DTO
* test(event-log): call-site registry as frozenset; drop dead pending machinery
* feat(event-log): scanner.stat_failed with emit-once discipline; drop dead vocabulary
* refactor(assets): drop API request failure events
* refactor(assets): drop ingest failure events
* refactor(assets): narrow event vocabulary to scan pipeline
* test(assets): per-call-site event tests collapse to mechanism-once
* refactor(assets): event log lines as logfmt, matching the log's own idiom
* test(assets): adapt hash-failure log assertion to privacy-safe scan-error logging
The rebase onto master picked up the privacy-safe _log_scan_error from
PR 16096, which no longer includes the file path in the log message.
Assert on the generic 'Asset scan error' message and that the path does
not leak, instead of asserting the path is present.
* fix(assets): enforce event vocabulary and scan counters
---------
Co-authored-by: Simon Pinfold <synap5e@users.noreply.github.com>
Co-authored-by: guill <jacob.e.segal@gmail.com>
* review-stack 1/4: code (37 files, +3217/-3958)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: path not under tests-unit/ or tests/
Question: Is the logic change right?
Source tip: 7007d18582
Merge-base: 783545f689
* review-stack 2/4: tests-removed (24 files, +274/-8220)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: test file deleted, or modified with deleted/(added+deleted) >= 0.9
Question: For each dropped assertion: obsolete by a ruling, or covered by a tests-new test?
Source tip: 7007d18582
Merge-base: 783545f689
* review-stack 3/4: tests-changed (13 files, +1043/-1218)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: remaining modified test files (incl. conftest.py / helpers)
Question: Did the edits weaken an existing check?
Source tip: 7007d18582
Merge-base: 783545f689
* review-stack 4/4: tests-new (46 files, +8601/-0)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: test file added
Question: Is the code layer well covered?
Source tip: 7007d18582
Merge-base: 783545f689
* review-stack 5/6: code (13 files, +351/-104)
Review-and-land stack for synap5e/feat/assets-di, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: path not under tests-unit/ or tests/
Question: Is the logic change right?
Source tip: eca2c74bff
Merge-base: 20d59d2a5f
* review-stack 6/6: tests (8 files, +753/-238)
Review-and-land stack for synap5e/feat/assets-di, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: every changed file under tests-unit/ or tests/ (added, modified, or deleted)
Question: Is the code layer well covered, and did any edit weaken an existing check?
Source tip: eca2c74bff
Merge-base: 20d59d2a5f
* review-stack 7/8: ported-fixes (42 files, +1361/-180)
Review-and-land stack for synap5e/feat/assets-di-v2, generated by
review-stack.py conventions (hand-built continuation layer; see the PR body).
Once approved, merges DOWN into the layer below (a fast-forward); only the
bottom layer squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: the 11 base-branch fix/docs commits 595cd6e4..94d7185b cherry-picked across the DI refactor (7efdd1d7 excluded, superseded by layer 8)
Question: was each base fix ported faithfully across the DI refactor?
Source tip: 6841881069284803b902b4a9e33bdcda13126771
Merge-base: 7fdfb40f4b
* review-stack 8/8: defensive-parity (4 files, +36/-3)
Review-and-land stack for synap5e/feat/assets-di-v2, generated by
review-stack.py conventions (hand-built continuation layer; see the PR body).
Once approved, merges DOWN into the layer below (a fast-forward); only the
bottom layer squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: match-or-improve master's dependency defenses — NoAssets selection when DB deps unavailable (7efdd1d7's outcome via the DI seam), requirements warning before assets imports, blake3 in the guarded dependency set
Question: does each degradation path now match or improve master's behavior?
Source tip: ebc2cfeebc
Merge-base: 7fdfb40f4b
* fix(assets): only discard content rows this operation actually inserted
CR-9: Enumerated all six create_content call sites. Only scanner seeding and the three ingest registration paths track IDs for failure cleanup.
* fix(assets): reject hash-only uploads with FEATURE_DISABLED when hashing is off
CodeRabbit finding CR-2: reject hash-only multipart uploads before create_from_hash when hashing is disabled.
* fix(assets): seed persists the stat it verified
CR-7: persist the fresh seed-time restat instead of walk-time spec values.
* fix(assets): route database lock failures to the lock guidance
CR-16: route file-lock startup failures through the existing lock guidance and exit path.
* fix(assets): drop the inaccurate temp-cleanup claim from the shutdown warning
References CR-10.
* fix(assets): walk the output root after execution so undeclared outputs register promptly
Custom nodes that write files into the output directory without declaring
them in output_ui only became assets when the next full walk happened - a
frontend GET /object_info or a restart. Headless and API-only sessions never
trigger either, so those files never converged into the asset database.
The post-execution hook now requests a FULL scan of the output root instead of
an enrich-only pass. The seeder's pending-request queue was generalised from
enrich-specific to carrying a scan phase, so the request starts immediately
when the seeder is idle and coalesces (escalating to FULL on a phase mismatch)
when a scan is already running. queue_output_enrichment is renamed to
queue_output_scan across the protocol, the NoAssets no-op and the call site.
References FIX-6.
* chore(assets): remove seeder paths orphaned by the output-scan change
45c2f96e rerouted both former enrich call sites to start()/enqueue_scan(),
leaving two seeder methods that look live but are not. Review round F2
raised this along with four smaller items; the user's disposition was to
fix all six here.
- Delete start_enrich: zero callers repo-wide after 45c2f96e.
- Delete enqueue_enrich: no production callers; its ~18 call sites in
tests/test_asset_seeder.py move to enqueue_scan(phase=ScanPhase.ENRICH)
with their semantics unchanged. The deletion forces the half-done class
renames (TestEnqueueEnrich* -> TestEnqueueScan*, consistent with the
already-renamed TestPendingScanDrain) and restores the module docstring
that was dropped rather than reworded.
- Document at manager.queue_output_scan that ScanPhase.FULL per debounce
window is the deliberate, user-ratified trade, so it is not optimised
back to ENRICH without revisiting the decision.
- Document that SeedAssetSpec.size_bytes/mtime_ns are walk-time
diagnostics only - production persists the seed-time restat since CR-7.
- Export create_content_reporting_insert from the queries facade and fold
scanner.py's direct-module import into the existing facade block.
- Harden test_queue_output_scan_does_not_duplicate_declared_output against
a vacuous pass: it now asserts the seeder finished without errors and
that an undeclared sibling written into the same directory WAS
registered by the same scan, proving the walk actually ran.
No production behaviour changes beyond the two deletions.
References F2-cleanup.
* chore: comment cleanup
Comment-Gate: 18 quarantined
* fix(assets): preserve pause across the seeder's pending-scan drain
pause() runs before every prompt, while pending-scan enqueue and resume only run inside the debounced gc-interval gate. If the active scan finishes just after the next prompt's pause, its finally block resets the seeder to idle and the pending drain starts a replacement with the run gate open, so resume becomes a no-op.
Capture pausedness under the lock before resetting to idle, then start the drained scan already paused. Setting the state and gate before launching the thread avoids the start-then-reclear window and lets resume release the existing scan checkpoints.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* test(assets): pin job_id absence for scan-discovered assets
Owner ruling, recorded 2026-09-03 in the stack-9-hardening planning notepad: scan-discovered assets — including undeclared outputs found by the post-execution walk — carry job_id = None, always; only emission-time registration (output_ui declaration) attributes a job; attributing walk finds to the most recent prompt would be a temporal-correlation guess that is wrong exactly when prompts interleave; None is honest provenance. Do NOT add proximity-based attribution heuristics to the scanner. Ratified against Jacob Segal's cross-job-attribution concern (2026-09-08 review meeting) — a wrongly-attributed asset could mean one user's cloud job sees another user's asset.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* [review-stack 10/10] assets-tests (#16218)
* test(execution): run the battery with assets enabled and assert asset-system health at teardown
* test(execution): cover list-shaped outputs registering assets
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
---------
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* [review-stack 11/11] review-fixes (#16261)
* fix(assets): only exit on database file-lock timeout when assets are enabled
* test(assets): pin live_contents_under_prefixes path-filtering semantics
* perf(assets): push live-content prefix filtering into SQL
* test(assets): declare per-entry intent in the path-prefix corpus
* test(assets): normalize POSIX-literal path expectations for Windows
* test(assets): force observable stat changes and close-before-mutate on Windows-sensitive rewrites
* test(assets): force an observable mtime change in the hash-mode split test
---------
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Co-authored-by: guill <jacob.e.segal@gmail.com>
* fix: respect user directory for default database
* refactor: use None default for --database-url instead of argv scan
Per review: default --database-url to None and treat a non-None value
as explicit at resolution time. Removes the sys.argv scan and the
database_url_explicit attribute. database_default_path now serves
directly as the legacy copy source. Adds regression tests for the
unchanged no-flag default path and explicit URLs at the legacy
location.
* refactor: rename legacy database to .bak after copy
Per review: after copying the legacy install-dir database to the
effective user directory, rename the original to comfyui.db.bak so a
later launch without --user-directory cannot silently fall back to a
diverged copy, while keeping the file around for recovery. Also hoist
the database_default_path import to module scope.
* fix: guard legacy migration on existing .bak and rename before copy
Per review: bail out of the legacy migration when comfyui.db.bak
already exists, so only the first run migrates and later launches with
a fresh --user-directory cannot grab a database another instance is
using. Rename before copy so os.replace fails fast if the legacy DB is
held open by a running instance.
---------
Co-authored-by: guill <jacob.e.segal@gmail.com>
preview_url was assembled from a /api/view link whose type was chosen by
matching the asset's tags against "input" then "output". Anything written
anywhere else - temp above all, where preview nodes put their images - fell
off the end of that chain and came back with no preview at all. Tags are
user-editable, so removing one also silently destroyed the URL.
Derive the URL from where the file actually sits instead. That covers every
root /api/view serves, temp included, and no longer depends on tags or on a
filename in user_metadata. A file outside those roots, or content no client
can render from its own bytes, gets no preview URL rather than one that
cannot work.
Nominated previews are resolved a page at a time rather than per row, so a
list costs one extra query however long it is. A preview that is soft-deleted
or not visible to the caller drops out of that lookup and is no longer
advertised.
Co-authored-by: guill <jacob.e.segal@gmail.com>
* Keep temp-directory assets visible while their files exist
Assets written to the temp directory were flagged as missing and dropped
from GET /api/assets, even with the file sitting on disk. One list of
directories was answering two different questions -- where the scanner
looks for new files, and which files ComfyUI considers its own -- and
temp belongs only in the second, so every temp reference was disowned by
the prune that runs at startup and on POST /api/assets/prune.
Ownership now covers temp. Discovery still does not: the temp directory
is wiped before the scan runs, and assets written there are already
registered with a hash, mime type and dimensions, so walking it would
find nothing. Temp references are instead reconciled against the
filesystem directly, so a temp file that really is gone is still retired
rather than lingering as a broken entry.
get_prefixes_for_root becomes get_scan_prefixes_for_root so the two
questions are told apart by name rather than by comment.
* Cover the unhashed temp asset in the reconciliation tests
The existing temp tests all registered hashed assets, so they never
exercised the path an unhashed asset takes when its file is gone: the
orphaned rows are removed rather than kept as missing, exactly as under
any other root.
* Implement tags_all/tags_any/tags_none on the assets list API (BE-6600)
Adds the three canonically-named tag filter params to GET /api/assets and
GET /api/assets/tags/refine:
- tags_all: asset carries every tag (replaces include_tags)
- tags_any: asset carries at least one tag (new)
- tags_none: asset carries no tag (replaces exclude_tags)
Clauses intersect; tags_none always wins. include_tags/exclude_tags remain
as permanent deprecated aliases and behave exactly as before when used on
their own.
Invalid combinations return 400 INVALID_TAG_FILTER, but only when the
request uses at least one new-name parameter (non-empty after
normalisation):
- mixed spellings of one slot (include_tags with tags_all, exclude_tags
with tags_none)
- the same tag in the effective all-list and none-list (query can never
match)
Old-names-only requests gain no new error paths: include_tags=a&exclude_tags=a
still returns an empty 200. tags_any/tags_none overlap stays valid (dead
term, not a dead query).
* Address review findings: positional-compat, deprecation metadata, test matrix
- Move any_tags to the end of the four touched signatures: inserting it
mid-signature silently misbound pre-existing positional callers (e.g.
a caller passing name_contains positionally would have it consumed as
any_tags).
- Mark include_tags/exclude_tags Field(deprecated=True) on both list
schemas so generated schema metadata matches the contract, not just a
comment (schemas_out.py already uses this form for Asset.name).
- Add tests: legal cross-slot old/new combinations, repeated query-key
concatenation (pins Core behavior; outside the cross-platform
contract), tags_any two-page cursor consistency (total/has_more/
no-overlap), refine-route mixed-spelling rejection + legacy-conflict
preservation, and schema deprecation metadata.
* Pin tag-value opacity: case-sensitive matching, byte-exact conflict check
The prod tag survey (~/comfy/prod-model-tag-shape.md) found live
case-distinct tag pairs (SEEDVR2/seedvr2) that resolve differently, so
the contract now states tag values are opaque byte-strings. Pin that:
case-distinct tags filter separately, and a case-distinct all/none pair
is not an INVALID_TAG_FILTER conflict.
* Document tags_all/tags_any/tags_none in openapi.yaml, deprecate aliases
Add the three tag-filter parameters to both listAssets and
getAssetTagHistogram parameter blocks and mark include_tags/exclude_tags
deprecated: true, keeping the spec in step with the runtime schemas so
generated clients can discover the new filters while the aliases stay
present for existing consumers.
* Move schemas_in import to module scope in test_list_filter
Review feedback: no import cycle requires the local import.
* Silence per-request DeprecationWarning in the tag-filter remap shim
Reading the deprecated include_tags/exclude_tags fields by attribute
fires pydantic's DeprecationWarning on every list/refine request even
for callers using only the new names. The warning is aimed at API
clients, not the server's own remap; read via model_dump instead.
* Cap tag-filter lists at 100 entries, all spellings
Review finding: unbounded tag lists fan out into one correlated EXISTS
per tag on both page and count statements. Cap each list at 100
normalized entries with 400 INVALID_TAG_FILTER naming the parameter.
Applies to the legacy spellings as well — a deliberate, decided
exception to the old-names-behave-identically rule, since a cap only on
new names would leave the same fan-out reachable through the aliases.
* Strip process narration from comments
Comments carried decision dates, contract cross-references, and review
context. Keep only the constraints the code cannot show, one line each.
This avoids name collision (circular imports) for external custom nodes,
for which the comfy path is pushed into sys.path so Python's own logging module
is shadowed otherwise.
fixes: #15229
* Fix SVG previews broken by the stored-XSS forced-download
/view and the assets download route force every SVG to
application/octet-stream + attachment. That blocks the stored XSS from
GHSA-779p-m5rp-r4h4, but it also breaks the SVG node output and Media
Assets previews, which request the file with a plain <img>.
Exempt only that case. An SVG referenced by an <img> loads in secure
static mode with scripting and external references disabled, so the
payload cannot fire. The attack needs the SVG to become a document,
which arrives with a different Sec-Fetch-Dest. Browsers set that header
themselves and page script cannot override it. A missing header, from a
non-browser client or a proxy that strips it, fails closed.
The blocklist itself is unchanged; this is a call-site gate.
* Don't let a cache replay the inline SVG into document context
The Sec-Fetch-Dest exemption makes /view and the assets content route vary
their Content-Type and Content-Disposition on a request header, but neither
response said so. FileResponse emits Last-Modified/ETag and the cache_control
middleware skips /view (the filename is in the query string, not the path), so
the inline image/svg+xml variant is heuristically cacheable. A cache keyed on
the URL alone could hand an entry primed by an <img> load to a later top-level
navigation of the same URL, turning the SVG back into a document and
re-enabling the stored XSS the forced download blocks.
Set Vary: Sec-Fetch-Dest and Cache-Control: no-store on both branches, not just
the exempt one: a cached attachment replayed to an <img> would re-break the
preview this fix exists to restore.
Also strip parameters from content_type before building the assets response.
mime_type there is uploader-supplied and unvalidated, and aiohttp rejects a
charset in the content_type argument with ValueError, so a stored
"image/svg+xml; charset=utf-8" turned a valid inline SVG into a 500.
Route-level guards now pin the headers on both branches and the parameterised
mime type; all three fail against the previous commit.
Each folder in the listing now carries its registered extension
allowlist verbatim; an empty array means the folder accepts any
extension (match-all), mirroring filter_files_extensions semantics.
Gives consumers the filtering rule itself rather than just its output:
/models/{folder} lists files by the per-folder rule but the rule is not
exposed anywhere, and /experiment/models/{folder} filters everything by
the global supported_pt_extensions regardless of registration.
Presentation-level filtering of match-all folders (e.g. hiding
README/config noise that repository-downloading custom nodes leave in
model directories) is deliberately left to the consumer.
Co-authored-by: guill <jacob.e.segal@gmail.com>
* feat(assets): add namespaced model type tags
* fix(assets): mark path-derived upload tags automatic
* fix(assets): merge duplicate scan specs
* test(assets): make duplicate path normalization portable
* feat(assets): add loader_path as the authoritative loader locator (#14796)
* fix(assets): filter model_type tags by bucket extension sets
Buckets sharing a base directory (e.g. diffusion_models and a custom
unet_gguf) tagged every file in the directory regardless of whether the
bucket could load it, so .safetensors files were tagged
model_type:unet_gguf and vice versa. Carry each bucket's registered
extension set through get_comfy_models_folders and only emit a
model_type tag when the file extension matches, keeping the empty-set
match-all convention from folder_paths.filter_files_extensions.
Files under a model base matching no bucket now keep only the models
tag instead of every directory-matching model_type tag.
* feat(assets): replace response file_path with persisted loader_path
The old file_path response field was a namespaced storage locator
(models/checkpoints/foo.safetensors): not an absolute path, not unique
identity, and not the value a loader consumes. Nothing needs that shape
on the wire (hash/ID-based locating is the long-term direction), so it
is dropped rather than renamed; the storage-root matching stays internal,
powering display_name.
What loaders DO need is the in-root loader path (category dropped:
models/checkpoints/foo/bar.safetensors -> foo/bar.safetensors). Serve it
as a first-class loader_path field, persisted on asset_references
(migration 0006) and written by every ingest pipeline at insert, so
responses read the column verbatim.
Like the model_type tags, loader_path is a seed-time derivative of the
model folder registry, maintained by the same scan lifecycle (new files seed
fresh values, pruning retires rows whose bucket disappeared). Rows
predating the column serve a null loader_path; databases from before
this stack already need recreating for the base branch's tag changes.
loader_path resolves every registered base including extra_model_paths
entries; display_name only the canonical storage roots. A file can
therefore be loadable with no display name (extra-path models) or the
reverse (unregistered files under the models root), and loader_path is
null exactly when no loader can resolve the file.
* test(assets): lock loader_path matrix (asymmetry, null, persist/read)
Cover the behaviour that has no production change but is easy to regress:
the extra-path asymmetry (loadable but no storage namespace), null
loader_path persistence for orphan files, and the response reading the
stored column with a compute fallback for un-backfilled rows.
* fix(assets): persist subfolder-qualified loader_path for ingested outputs
ingest_existing_file built its seed spec with the file's basename, so
outputs saved into a subfolder persisted loader_path (and the
user_metadata filename that preview URLs split for their subfolder
param) as just the basename: the served locator pointed at a file that
does not exist at that path. Scanner and seeder specs already derive
fname via compute_loader_path; use the same derivation here.
* fix(assets): only extension-matching buckets contribute a loader_path
The model-base match in get_asset_category_and_relative_path ignored
each bucket's extension set, so a file inside a registered base whose
extension the bucket cannot load (e.g. a .txt uploaded into
model_type:checkpoints) advertised a loader_path that no loader list
would ever resolve, while the tag side of the same stack already
excluded it. Apply the extension check used for backend tags (empty set
accepts any extension), keeping loader_path null exactly when no loader
can resolve the file.
* fix(assets): refresh loader_path when re-ingesting an existing reference
upsert_reference only wrote loader_path on the INSERT branch, so
re-ingesting an existing reference (an output overwritten in place, or a
file re-registered after its loader_path derivation changed) kept the
stale or NULL value forever. Write it on the UPDATE branch too, with a
null-safe change guard so a loader_path difference alone is enough to
trigger the update, and identical values stay a no-op.
* fix(assets): repair semantic merge breakage from #14796 and master
Two textually-clean but semantically-broken merges:
- routes.py lost its folder_paths import when #14796's import block
superseded the base's, while the content-type hardening added via the
base's master merge still calls folder_paths.is_dangerous_content_type.
- master's SVG download-hardening test uploads with the pre-namespacing
bare checkpoints tag, which this branch's destination validation
rejects; use model_type:checkpoints.
---------
Co-authored-by: guill <jacob.e.segal@gmail.com>
* security: fix five vulnerabilities (GHSA-779p-m5rp-r4h4)
- CVE-2026-56670: force download of SVG/XML responses on /view to prevent stored XSS
- CVE-2026-56671: contain /experiment/models/preview reads within the model folder
- CVE-2026-56672: stop inline rendering of uploaded /userdata/{file} content
- CVE-2026-56673: prevent path traversal in get_annotated_filepath (LoadImage /prompt input)
- CVE-2026-56674: reject opaque/null Origin to close the CSRF middleware bypass
Adds regression tests under tests-unit/security_test/ covering all five.
* security: address review feedback on GHSA-779p fixes
- Fix Windows CI failure in test_get_annotated_filepath: compare against
os.path.abspath(...) to match the intentional abspath normalization added
by the traversal hardening (abspath prepends the drive letter on Windows).
- origin_check: narrow the bare `except:` in is_loopback() to ValueError so
genuine interrupts aren't swallowed (review nit).
- origin_check: guard .port access in is_cross_origin_forbidden() so a
malformed/out-of-range port (e.g. Origin: http://127.0.0.1:99999) fails
closed with a 403 instead of surfacing an uncaught 500 in the middleware.
- server /view: escape backslash/quote in the Content-Disposition filename
(RFC 6266 quoted-string) so a filename containing a double quote can't
malform the response header.
* security: address CodeRabbit review feedback on GHSA-779p tests
- test #3: guard the symlink-escape test with a try/except skip so it no
longer errors on Windows CI where os.symlink needs elevated privileges /
Developer Mode (mirrors the guard in the sibling test #2).
- test #5: refresh the stale module docstring to describe the actual /view
gating (view_image closure calling folder_paths.is_dangerous_content_type,
the normalising check) instead of the bypassable raw set-membership test.
* revert(security): drop CVE-2026-56674 Origin: null CSRF change
Per maintainer review, the reported CSRF is already mitigated by the pre-existing
Sec-Fetch-Site: cross-site check for current browsers, and the null-origin
rejection risked breaking legitimate sandboxed-iframe embeds. Restores
origin_only_middleware and is_loopback in server.py to their prior state
(the Sec-Fetch-Site check is retained) and removes utils/origin_check.py and its
regression test. The other four GHSA-779p fixes are unaffected.
The job_ids query filter added in #13998 has no live consumer: the
frontend Generated tab kept sourcing from GET /jobs, and the cloud side
removed its equivalent filter from the shared asset spec. Carrying it on
the local server only re-introduces Core<->Cloud drift on the shared
contract, so remove it to match.
Removed: the job_ids field + validator on ListAssetsQuery, the IN(...)
clauses in list_references_page, the service/route passthrough, and the
filter-only tests.
Kept: the canonical-UUID prompt_id enforcement at job creation (also
landed in #13998). It stands on its own -- job ids are matched verbatim
by history keys, websocket correlation, and /interrupt -- and cloud
inherits it by running core for execution, so no divergence is created.
* feat(assets): add job_ids filter to GET /api/assets
Mirrors the existing cloud `job_ids` query param on the local Python server:
clients can pass a comma-separated list (or repeated query params) of UUIDs
to filter assets by their associated job.
The `AssetReference.job_id` column already exists, so no migration is
needed — this just plumbs the filter through schema → service → query.
Marks the parameter as available in both runtimes by dropping the
`[cloud-only]` description prefix and the `x-runtime: [cloud]` tag from
the OpenAPI spec, per the OSS field-drift convention (absent runtime tag
= populated by both local and cloud).
* fix(assets): tighten job_ids — array schema, max_length, narrow except
From cursor-reviews on the parent commit:
- OpenAPI: declare job_ids as `type: array, items: string format: uuid`
with `style: form, explode: true` so it matches the documented
contract (and matches sibling include_tags/exclude_tags shape).
Description now states both accepted shapes explicitly.
- Schema: cap `job_ids` at 500 entries (max_length on the Pydantic
field) so a client can't splice an unbounded list into the IN clauses.
- Schema: drop `AttributeError` from the except — `raw` only contains
`str` items by construction, so `uuid.UUID(<str>)` raises `ValueError`
exclusively; the second clause was dead code.
* fix(assets): tighten job_ids validator + add schema-level tests
Aligns with the parallel hardening from draft PR #13848 (now closed as
a duplicate). The validator now:
- Raises ValueError on non-string list items (was: silently dropped).
- Raises ValueError on non-string / non-list top-level values like dict
or int (was: silently passed through to Pydantic's downstream coercion).
Adds tests-unit/assets_test/queries/test_list_assets_query.py covering
the validator end-to-end: CSV canonicalization, dedup order, default
empty, invalid UUID, non-string list item, non-string non-list value,
and the max_length=500 boundary.
* feat(prompt): enforce canonical UUID prompt_id at job creation
POST /prompt previously accepted any client-supplied prompt_id verbatim,
str()-coercing even non-strings, and minting the literal job id "None"
for an explicit JSON null. The new GET /api/assets job_ids filter matches
stored job ids as canonical UUIDs exactly, so a non-UUID id minted a job
whose assets could never be filtered.
- validate_job_id (comfy_execution/jobs.py): requires a string in the
canonical lowercase hyphenated UUID form; raises ValueError otherwise,
including parseable-but-non-canonical spellings (uppercase, braced, URN,
bare hex), which would otherwise be silently rewritten and then miss
every exact-match lookup downstream (history keys, websocket
correlation, /interrupt, the assets job_ids filter).
- POST /prompt: absent or null prompt_id means the server mints uuid4;
invalid means 400 invalid_prompt_id on the standard error envelope.
- openapi.yaml: document the request-side prompt_id (format uuid,
nullable) on PromptRequest.
- tests: unit matrix for validate_job_id; integration tests against the
booted server covering rejection, acceptance, and null handling.
---------
Co-authored-by: guill <jacob.e.segal@gmail.com>
* fix(assets): remove unused delete_content param from deleteAsset
The delete_content query param on DELETE /api/assets/{id} was introduced
in #12125 and had its default flipped to false in #12621. In practice no
client sends it: the frontend issues a bare DELETE /assets/{id}, so every
real caller already gets the default soft-delete (the reference is hidden,
content preserved). The only thing that set delete_content=true was this
repo's own test teardown.
Remove the param from the route and the OpenAPI spec so the contract
matches what clients actually use (and lines up with the cloud surface).
The route now always soft-deletes. The underlying delete_asset_reference
helper keeps its delete_content_if_orphan option, so orphan reclamation
remains available internally for a future GC path — it's just no longer
exposed on the public endpoint. Tests that used delete_content=true for
hard cleanup now soft-delete; test_delete_upon_reference_count asserts
content preservation instead of orphan removal.
* test/docs: address review on deleteAsset delete_content removal
- Rename test_delete_upon_reference_count ->
test_soft_delete_preserves_asset_identity_across_references; the old name
implied last-ref cleanup, but it now verifies the opposite (soft delete
preserves identity across references).
- Strengthen the re-association assertion: also check asset_hash == src_hash
so it proves content reuse rather than relying on the now-tautological
created_new is False.
- Document delete_asset_reference: the orphan-reclamation branch is
intentionally internal-only; the public endpoint always soft-deletes.
- Normalize the soft-delete comment phrasing.
* test(assets): make seed content unique per test for isolation
Removing the delete_content param means delete is always a soft delete, so
content created by one test now survives into the next. The suite had been
relying on hard-delete teardown for isolation, so shared fixed-content
fixtures started colliding: seeded_asset (b"A"*4096) and
make_asset_bytes (deterministic on name) produced the same hash every test,
so the second seed deduped to the surviving asset and returned 200 instead
of 201, cascading into ~14 failures/errors.
Salt both fixtures with a per-test uuid so each test creates fresh content
(created_new True, 201), while keeping content deterministic within a test
(same name/size -> same bytes) and preserving exact byte length so size-based
list/sort assertions are unaffected.
* spec(assets): add cursor pagination params to GET /api/assets
Add 'after' query param and 'next_cursor' response field for keyset
pagination. Matches the cloud Go implementation (BE-893) so frontend
sees a unified contract across runtimes. Offset/limit remain as a
deprecated fallback.
* feat(assets): add cursor encode/decode helpers for keyset pagination
Port of cloud common/pagination/cursor.go. Wire format is base64url of
{"s", "v", "id"} JSON; times are Unix microseconds UTC to match
PostgreSQL timestamp precision.
Includes a byte-identity fixture pinned against the cloud Go wire
format so cross-runtime FE pagination can't silently drift.
* feat(assets): thread cursor through schemas, service, and query layer
list_assets_page accepts an opaque 'after' cursor and returns
next_cursor when more pages are available. The query applies a keyset
WHERE clause and a secondary ORDER BY id for deterministic tiebreak.
Cursor sort field is validated against the request sort, and a
last_access_time sort (OSS-only) falls back to offset/limit. Offset is
ignored whenever a cursor is supplied.
* feat(assets): wire cursor pagination through GET /api/assets handler
Adds integration tests for: full cursor walk, invalid-cursor 400,
sort/cursor mismatch 400, cursor-wins-over-offset, absent next_cursor
when no more results, and pagination stability across deletes.
* fix(assets): address cursor-review verified findings
- Mint next_cursor on every cursor-supported sort, not only when 'after'
was supplied. A first request (no 'after') previously returned
next_cursor=None, leaving cursor mode unreachable from a clean start.
- Over-fetch limit+1 so an exactly-full terminal page doesn't mint a
spurious cursor pointing at a phantom next page.
- Map crafted out-of-range microsecond cursors (OverflowError / OSError
in datetime construction) to 400 INVALID_CURSOR instead of leaking 500.
- Bump MAX_CURSOR_VALUE_LENGTH 256 -> 512 to match the AssetReference
name column max; without this, a long-named asset minted a cursor the
same server then refused on the next request. Cross-runtime byte
identity with cloud is unaffected because no cloud cursor ever carries
a value > 256 (cloud schema doesn't permit it).
- Return None from _encode_next_cursor when the boundary row carries a
NULL sort value (e.g. an Asset without size_bytes backfilled), instead
of silently encoding 0 and mis-positioning the keyset.
- Fix schemas_in.py comment so it matches actual handler behavior
(last_access_time + 'after' raises 400, does not fall back).
- Add AssetsApiError schema + 400 response to GET /api/assets in
openapi.yaml so generated clients know the INVALID_CURSOR envelope.
- Extend integration coverage: first-page mint, exact-multiple terminal
page, cursor walks for created_at/updated_at/size sorts, datetime
overflow surfaces as 400 not 500.
- Add unit coverage for datetime overflow and 512-char round-trip.
* feat(assets): bind cursor to sort order + Go-compat JSON escaping
Address three needs-judgment items from the cursor-review judge synthesis:
1. Cursor wire format now includes an "o" key carrying the sort
direction ("asc" / "desc") it was minted under. A request that
replays the cursor with a flipped `order` parameter is rejected
with 400 INVALID_CURSOR instead of silently walking the wrong
direction. Legacy cursors without "o" still decode (the binding
is best-effort until cloud mirrors the field — follow-up filed
separately).
2. JSON serialization now escapes `<`, `>`, `&`, U+2028, U+2029
to mirror Go's default `json.Marshal` behavior. Without this, an
asset name containing those characters produced different bytes on
Python vs cloud Go. The escaped form is what both runtimes emit.
3. Add direct query-layer tests for the keyset tiebreaker — the secondary
ORDER BY id branch was previously unexercised. Two scenarios: all
rows share a primary sort value, and mixed ties straddle page
boundaries. Both assert no row is dropped or duplicated across the
walk.
Wire-format note: Python cursors now differ from current cloud cursors
by exactly the "o" key. Cloud follow-up will bring the two back into
byte alignment.
* fix(assets): address bot review comments
- Soften offset param prose: it's not deprecated, just not preferred for
sequential walks. Random-access UIs (jump-to-page, item count displays)
legitimately still want offset, so dropping the 'deprecated' framing
rather than promoting it to a machine-readable deprecated:true flag.
- Add explicit HTTP status assertions before every json() / next_cursor
read in test_list_cursor.py so a failing request surfaces as an HTTP
error instead of a confusing KeyError on a 4xx/5xx body.
* feat(assets): require cursor o field, drop legacy permissive path
Cursor pagination hasn't shipped on either runtime yet — this PR is
still draft and cloud's mirror is just behind it — so there are no
legacy no-o cursors in the wild. Make o mandatory from day one
rather than landing permissive and tightening later.
decode_cursor now rejects any payload without o (or with a non-string
o) as malformed. CursorPayload.order becomes a required str. Tests
that constructed CursorPayload directly now pass order="desc";
test_legacy_cursor_without_order_accepted flips to
test_cursor_without_order_rejected.
* chore(assets): drop cross-repo prose from cursor comments
Strip prose references to sibling Go implementations and external
ticket IDs from cursor.py, the cursor tests, the keyset integration
tests, asset_management's sort-field comment, and the legacy
prompt_id alias comment. Pure docstring/comment scrub — no behavior
or wire-format changes. x-runtime: [cloud] field annotations in
openapi.yaml are unchanged; those are the spec's structural
cross-runtime convention, not internal references.
* test(assets): include 'o' in microsecond-boundary cursor payload
The boundary test was building a cursor without the required `o` key, so
decode failed on the missing-order branch before reaching the µs-overflow
path the test is asserting. Both paths return 400 INVALID_CURSOR so the
assertion passed for the wrong reason. Add `o` to the payload and matching
`order=` to the request so the decode reaches the intended branch.
* fix(assets): address ultrareview findings on cursor pagination
Six fact-checked findings from the multi-model review pass:
- Encoder/decoder length asymmetry: encode_cursor now rejects empty id,
oversized id (>128), oversized value (>512), and invalid order tokens
symmetrically with decode_cursor. Prevents the same server from minting
a cursor it then 400s on the next request (e.g. a filesystem-scanned
asset name >512 chars). The bad-order path now raises InvalidCursorError
(still subclasses ValueError) so route-layer handling stays uniform.
- Raw U+2028/U+2029 in cursor.py source: ripgrep treated those lines as
line-terminators, confirming the bytes were the actual separators. Any
editor save / autoformat / git tooling that normalizes invisibles would
silently break the encoder. Replaced with explicit /
Python escape sequences.
- set(seen) == set(names) hid ordering regressions: a cursor walk that
dropped a row at a page boundary or returned duplicates could pass.
Reworked the assertion to (1) reject duplicates, (2) require full
coverage, and (3) assert strict positional order for size sort, the
only field with a clock-independent ordering.
- Flaky time.sleep(0.05) between inserts: Windows CI clock resolution is
~15ms, so back-to-back inserts under load could collide and exercise
the tiebreaker instead of the documented path. Removed the sleep and
let the strengthened assertion above carry coverage / no-duplicates,
with size sort carrying strict order.
- Cursor error envelope diverged from the rest of routes.py: cursor 400s
emitted {error: {code, message}} while every other 400 in the file
emits {error: {code, message, details}} via _build_error_response.
Switched to _build_error_response and added the details field to the
AssetsApiError schema in openapi.yaml.
- "Byte-identity fixtures" only checked substring containment, defeating
the test class's stated purpose of pinning the wire format. Switched
to exact-bytes equality against an inline expected payload string per
fixture, so any whitespace / key-order / escape drift fails loudly.
Also dropped Go / json.Marshal references from docstrings — the byte
format is the contract, not the runtime that mints it.
* fix(assets): cap cursors by encoded wire size, not just char count
Char-count guards on value/id can still let multibyte or escape-heavy
inputs blow past MAX_ENCODED_CURSOR_LENGTH once UTF-8 + escape expansion
+ base64url runs. A 512-character name of 'é' (2 bytes UTF-8) or '<'
(serializes to the 6-byte '<' escape) passes the char check, mints
a ~1500-byte cursor, then 400s when handed back on the next request.
Compute the final encoded form and reject it before returning if it
exceeds the wire cap. Adds regression tests for both inflation paths.
* refactor(assets): extract cursor JSON escaping helper; size wire cap above per-field caps
Addresses review feedback on cursor.py:
- Extract the inline escape chain into _apply_wire_compatible_json_escapes()
with a comment pinning it to the wire format's escape set, so the parity
intent is explicit rather than reading as an ad-hoc transform.
- Raise MAX_ENCODED_CURSOR_LENGTH to 8192 (comfortably above the ~5.2KB
worst-case the per-field caps can produce) and drop the mint-time length
guard. Encoder/decoder symmetry now holds by construction: the encoder
can't produce a cursor the decode path rejects, so there is no confusing
user-visible 'cursor too long' failure at mint time.
- Rewrite the two over-wire-cap tests to assert worst-case multibyte and
escape-heavy values mint and round-trip, instead of being rejected.
* refactor(assets): drop cross-runtime cursor escaping; cursors are opaque
The custom JSON escaping of <, >, &, U+2028, and U+2029 existed only to
keep the encoded cursor byte-identical with the Cloud implementation of
the same payload format. Cursors are opaque tokens, so byte-level
compatibility across implementations is not needed — plain json.dumps
output is sufficient. Remove the escaping helper and the byte-identity
test fixtures that pinned the wire format; keep round-trip coverage for
the affected characters.
---------
Co-authored-by: guill <jacob.e.segal@gmail.com>
Simon Pinfold