mirror of
https://github.com/brunopostle/ifcurl.git
synced 2026-07-12 10:18:14 +00:00
render.py: cap worker count; auth.py: document token leak; README: fix example
- render.py: replace bare cpu_count() with _WORKER_COUNT = min(cpu_count, 4), configurable via IFCURL_RENDER_WORKERS env var; prevents N concurrent requests from spawning N×cpu_count geometry worker processes - auth.py: document that inject_token() embeds the token in git CLI arguments, making it visible in the process list; note SSH as the workaround - README: replace placeholder comment with actual tempfile.mkstemp pattern for loading IFC bytes into ifcopenshell (which requires a file path, not bytes) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
ee317bef75
commit
19ce25fdc0
4 changed files with 35 additions and 9 deletions
|
|
@ -47,9 +47,9 @@
|
|||
{"id":"ifcurl-4xl","title":"viewer.html: selector support is a subset of IfcOpenShell syntax — not documented","description":"applySelector() in the viewer only handles type-name union syntax ('IfcWall+IfcSlab'). The comment says 'Attribute/property filters not yet supported', but this limitation is not visible to users and not documented in README.md or SPECIFICATION.md. A user pasting a URL with 'selector=IfcWall,+Pset_WallCommon.FireRating=2HR' will see all elements rather than the filtered set, with no error. Should add a user-visible note or at minimum document the viewer's selector limitations.","status":"closed","priority":3,"issue_type":"task","assignee":"Bruno Postle","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:49Z","created_by":"Bruno Postle","updated_at":"2026-04-24T14:04:57Z","started_at":"2026-04-24T13:22:10Z","closed_at":"2026-04-24T14:04:57Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-fu5","title":"service.py: no rate limiting or request size cap on /preview and /bcf","description":"The preview and BCF endpoints have no rate limiting, no cap on IFC file size fetched, and no timeout on the render operation. A single large IFC file or a flood of requests can saturate memory and CPU. Consider: max IFC blob size (reject fetch if commit object too large), render timeout via threading.Timer, and a simple in-process rate limiter or documentation that a reverse proxy (nginx) should front the service with rate limits.","status":"open","priority":3,"issue_type":"task","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:45Z","created_by":"Bruno Postle","updated_at":"2026-04-24T10:41:45Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-d58","title":"ifc_url_test.go: no test for bare URL with path containing @ character","description":"The Go transformer reassembles multi-sibling text nodes for bare ifc:// URLs. There is a test for underscores in the path (which goldmark splits on), but no test for paths or query values containing '@', which is a valid character in query strings and could appear in selector expressions. Also missing: test for a bare URL followed immediately by punctuation like a period or closing parenthesis to verify the terminator logic.","status":"closed","priority":3,"issue_type":"task","assignee":"Bruno Postle","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:32Z","created_by":"Bruno Postle","updated_at":"2026-04-24T14:04:56Z","started_at":"2026-04-24T13:22:09Z","closed_at":"2026-04-24T14:04:56Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-7mg","title":"auth.py: token appears in git process arguments, visible in process list","description":"inject_token() embeds the authentication token into the HTTPS remote URL as 'https://\u003ctoken\u003e@host/path'. This URL is passed directly to git clone/fetch as a command-line argument, which means the token is visible in the process list (ps aux) while the git command runs. On a shared server this is a security leak. The standard mitigation is to use GIT_ASKPASS or a credential helper that supplies the token via stdin/environment rather than via the URL. Should be documented as a known limitation at minimum.","status":"open","priority":3,"issue_type":"bug","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:29Z","created_by":"Bruno Postle","updated_at":"2026-04-24T10:41:29Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-z4f","title":"render.py: concurrent requests each spawn full CPU-count process pools","description":"render() calls ifcopenshell.geom.iterator(settings, model, multiprocessing.cpu_count(), ...). In a multi-request service scenario, N concurrent renders each spawn cpu_count worker processes, so a 16-core machine handling 4 simultaneous requests spawns 64 processes. This can saturate the machine and is effectively a DoS amplifier. Consider capping the worker count (e.g. max 2 or 4 per request), or using a shared process pool at the service level.","status":"open","priority":3,"issue_type":"bug","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:16Z","created_by":"Bruno Postle","updated_at":"2026-04-24T10:41:16Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-5or","title":"README.md: library usage example doesn't show temp file pattern for ifcopenshell.open()","description":"The 'Use as a library' section shows 'model = ifcopenshell.open(...) # write ifc_bytes to tmp file first' with a comment but no actual code. ifcopenshell.open() requires a file path, not bytes. A new user following the README will be stuck. The example should show the tempfile.mkstemp pattern used in _load_model() in service.py, or at least reference that function.","status":"open","priority":3,"issue_type":"task","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:04Z","created_by":"Bruno Postle","updated_at":"2026-04-24T10:41:04Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-7mg","title":"auth.py: token appears in git process arguments, visible in process list","description":"inject_token() embeds the authentication token into the HTTPS remote URL as 'https://\u003ctoken\u003e@host/path'. This URL is passed directly to git clone/fetch as a command-line argument, which means the token is visible in the process list (ps aux) while the git command runs. On a shared server this is a security leak. The standard mitigation is to use GIT_ASKPASS or a credential helper that supplies the token via stdin/environment rather than via the URL. Should be documented as a known limitation at minimum.","status":"closed","priority":3,"issue_type":"bug","assignee":"Bruno Postle","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:29Z","created_by":"Bruno Postle","updated_at":"2026-04-24T15:52:13Z","started_at":"2026-04-24T14:47:34Z","closed_at":"2026-04-24T15:52:13Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-z4f","title":"render.py: concurrent requests each spawn full CPU-count process pools","description":"render() calls ifcopenshell.geom.iterator(settings, model, multiprocessing.cpu_count(), ...). In a multi-request service scenario, N concurrent renders each spawn cpu_count worker processes, so a 16-core machine handling 4 simultaneous requests spawns 64 processes. This can saturate the machine and is effectively a DoS amplifier. Consider capping the worker count (e.g. max 2 or 4 per request), or using a shared process pool at the service level.","status":"closed","priority":3,"issue_type":"bug","assignee":"Bruno Postle","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:16Z","created_by":"Bruno Postle","updated_at":"2026-04-24T15:52:14Z","started_at":"2026-04-24T14:47:54Z","closed_at":"2026-04-24T15:52:14Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-5or","title":"README.md: library usage example doesn't show temp file pattern for ifcopenshell.open()","description":"The 'Use as a library' section shows 'model = ifcopenshell.open(...) # write ifc_bytes to tmp file first' with a comment but no actual code. ifcopenshell.open() requires a file path, not bytes. A new user following the README will be stuck. The example should show the tempfile.mkstemp pattern used in _load_model() in service.py, or at least reference that function.","status":"closed","priority":3,"issue_type":"task","assignee":"Bruno Postle","owner":"bruno@postle.net","created_at":"2026-04-24T10:41:04Z","created_by":"Bruno Postle","updated_at":"2026-04-24T15:52:14Z","started_at":"2026-04-24T14:47:55Z","closed_at":"2026-04-24T15:52:14Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-dyy","title":"viewer.html: BCF export from viewer omits component GUIDs — undocumented","description":"The viewer's client-side BCF export cannot include component GUIDs because it has no access to the IFC model's resolved GlobalIds. The service POST /bcf endpoint resolves the selector server-side and produces proper BCF with \u003cSelection\u003e or \u003cVisibility/Exceptions\u003e elements. This discrepancy is not documented anywhere. The viewer's BCF button should either show a note ('GUIDs not resolved — use /bcf API for full component selection') or redirect to the service endpoint when available.","status":"open","priority":3,"issue_type":"task","owner":"bruno@postle.net","created_at":"2026-04-24T10:40:49Z","created_by":"Bruno Postle","updated_at":"2026-04-24T10:40:49Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-tq8","title":"No tests for ifcurl cache CLI subcommands","description":"The 'ifcurl cache list', 'ifcurl cache prune', and 'ifcurl cache clear' subcommands in __main__.py have no automated test coverage. _repo_cache_entries(), _evict_if_needed(), and _get_max_cache_bytes() have unit tests in test_git.py, but the CLI layer (_cmd_cache) is untested. Should add tests that exercise the argparse wiring, the output formatting, and the prune logic via subprocess or by calling _cmd_cache() directly with a fake args namespace.","status":"open","priority":3,"issue_type":"task","owner":"bruno@postle.net","created_at":"2026-04-24T10:40:45Z","created_by":"Bruno Postle","updated_at":"2026-04-24T10:40:45Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
{"id":"ifcurl-ovi","title":"dev/compose.yml uses Forgejo v10, but go.patch targets v13","description":"dev/compose.yml pulls codeberg.org/forgejo/forgejo:10 for local development. The go.patch and ifc_url.go were written and tested against the v13 branch. Running the dev environment with v10 won't have the patched binary or the correct template slots. The compose file should either use the patched v13 build, or document that the stock container is for testing the service/viewer in isolation only, not for testing the Go patch.","status":"open","priority":3,"issue_type":"bug","owner":"bruno@postle.net","created_at":"2026-04-24T10:40:35Z","created_by":"Bruno Postle","updated_at":"2026-04-24T10:40:35Z","dependency_count":0,"dependent_count":0,"comment_count":0}
|
||||
|
|
|
|||
12
README.md
12
README.md
|
|
@ -88,6 +88,8 @@ ifcurl render "ifc://..." -o output.png
|
|||
Use as a library:
|
||||
|
||||
```python
|
||||
import os, tempfile
|
||||
import ifcopenshell
|
||||
from ifcurl import IfcUrl, fetch_ifc
|
||||
from ifcurl.render import render
|
||||
|
||||
|
|
@ -96,7 +98,15 @@ url = IfcUrl.parse(
|
|||
"?path=models/building.ifc&selector=IfcWall&visibility=ghost"
|
||||
)
|
||||
hexsha, ifc_bytes = fetch_ifc(url) # hexsha is stable even for mutable refs
|
||||
model = ifcopenshell.open(...) # write ifc_bytes to tmp file first
|
||||
|
||||
# ifcopenshell.open() requires a file path, not bytes
|
||||
tmp_fd, tmp_path = tempfile.mkstemp(suffix=".ifc")
|
||||
try:
|
||||
os.write(tmp_fd, ifc_bytes)
|
||||
os.close(tmp_fd)
|
||||
model = ifcopenshell.open(tmp_path)
|
||||
finally:
|
||||
os.unlink(tmp_path)
|
||||
|
||||
png = render(model, selector=url.selector, clips=url.clips or None,
|
||||
camera=url.camera, fov=url.fov, visibility=url.visibility)
|
||||
|
|
|
|||
|
|
@ -69,8 +69,16 @@ def inject_token(https_url: str, token: str) -> str:
|
|||
``https://host/org/repo`` → ``https://<token>@host/org/repo``
|
||||
|
||||
This format is accepted by GitHub, GitLab, Gitea, and most other git
|
||||
hosting platforms. The token is never stored — it only appears in the
|
||||
git command line arguments used for clone/fetch operations.
|
||||
hosting platforms. The token is never stored on disk — it only lives
|
||||
in the authenticated URL passed to git clone/fetch as a command-line
|
||||
argument.
|
||||
|
||||
**Known limitation**: the token is visible in the OS process list
|
||||
(``ps aux``) for the duration of the git subprocess. On shared servers
|
||||
this is a potential credential leak. The standard mitigation is to use
|
||||
``GIT_ASKPASS`` or a credential helper, which this implementation does
|
||||
not yet do. As a workaround, configure SSH transport (``git@host:…``)
|
||||
rather than HTTPS for private repositories on shared hosts.
|
||||
"""
|
||||
parsed = urlparse(https_url)
|
||||
port_suffix = f":{parsed.port}" if parsed.port else ""
|
||||
|
|
|
|||
|
|
@ -33,6 +33,14 @@ import multiprocessing
|
|||
import os
|
||||
import tempfile
|
||||
|
||||
# Cap parallel geometry workers per render call. In service deployments,
|
||||
# N concurrent requests × cpu_count workers each can saturate the host.
|
||||
# Override with IFCURL_RENDER_WORKERS (e.g. "8" for a dedicated render host).
|
||||
_WORKER_COUNT: int = min(
|
||||
multiprocessing.cpu_count(),
|
||||
int(os.environ.get("IFCURL_RENDER_WORKERS", "4")),
|
||||
)
|
||||
|
||||
import ifcopenshell
|
||||
import ifcopenshell.geom
|
||||
import ifcopenshell.guid
|
||||
|
|
@ -327,7 +335,7 @@ def _render_with_types(
|
|||
include.extend(tmp.by_id(e.id()) for e in selector_elements)
|
||||
|
||||
settings = _build_geom_settings(tmp)
|
||||
iterator = ifcopenshell.geom.iterator(settings, tmp, multiprocessing.cpu_count(), include=include)
|
||||
iterator = ifcopenshell.geom.iterator(settings, tmp, _WORKER_COUNT, include=include)
|
||||
if not iterator.initialize():
|
||||
raise ValueError("Type entities have no renderable geometry")
|
||||
|
||||
|
|
@ -436,12 +444,12 @@ def render(
|
|||
|
||||
if selector_elements is not None and visibility == "isolate":
|
||||
iterator = ifcopenshell.geom.iterator(
|
||||
settings, model, multiprocessing.cpu_count(), include=selector_elements
|
||||
settings, model, _WORKER_COUNT, include=selector_elements
|
||||
)
|
||||
else:
|
||||
exclude = list(model.by_type("IfcOpeningElement"))
|
||||
iterator = ifcopenshell.geom.iterator(
|
||||
settings, model, multiprocessing.cpu_count(),
|
||||
settings, model, _WORKER_COUNT,
|
||||
exclude=exclude if exclude else None,
|
||||
)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue