Further fixes to job submission

[allison-truhlar]

This PR troubleshoots submitting jobs to the Janelia cluster when running the Fileglancer server as root.

Changes so far:

1. Work directory fix - Resolved ~ to the actual user's home (~username), not root's home.
2. seteuid job submission - Wrapped work dir creation, symlink, and executor.submit() in the EffectiveUserContext so filesystem operations happen as the user.
3. Stale symlink fix - Unlink existing repo symlink before creating a new one (caused an issue when a job failed - the symlink wasn't removed).
4. bsub -U username flag — Because bsub uses getuid() (real UID = root) not geteuid(), we pass -U username to tell LSF to run the job as the actual user. Note: unsure whether this is actually needed after adding the missing FGC_CLUSTER__EXECUTOR=lsf to the fileglancer-hub .env file — should test removing it once everything else is fixed
5. cluster.extra_paths config option — Prepends directories to os.environ["PATH"] at server startup so bsub/bjobs/bkill are findable. Needed because systemd services have minimal PATH. Documented in config.yaml.template.

Where we are now (as of release 2.7.0a7, commit eff3c09)

• bsub is found on PATH (no more FileNotFoundError), but it fails because LSF can't find its configuration

@krokicki

In general, to fix “uncontrolled data used in path expression” here, we need to ensure that any path derived from username cannot escape a controlled root directory. The standard pattern is: (1) compute a base directory that is not influenced by untrusted input, (2) build any subpaths under this base using path-joining, (3) normalise (resolve() / os.path.normpath) and verify that the resulting path is still anchored in the base directory. If the check fails, reject the operation.

For this code, the main problem is _build_work_dir, which currently uses home = os.path.expanduser(f"~{username}") and then returns a Path directly built from that. The cleanest fix that does not change external functionality is:

• Define a fixed jobs root directory, e.g. _JOBS_BASE = Path(os.path.expanduser("~/.fileglancer/jobs")), which is already the conceptual location we use.
• In _build_work_dir, derive home using the existing logic (so we still respect the username’s home directory), but then build the work directory as a subpath under _JOBS_BASE, not directly from home. For example, create a per-user subdirectory _JOBS_BASE / _sanitize_for_path(username) and then append the job-specific suffix.
• Alternatively, if you must preserve the idea “jobs go under each user’s home”, you can still normalise and enforce that the final work_dir path is within that user’s home directory: compute base = Path(home).expanduser().resolve(), then work_dir = (base / ".fileglancer" / "jobs" / suffix).resolve(), and finally check if base not in work_dir.parents and base != work_dir: raise ValueError(...). This ensures no traversal outside the home directory even if username contains weird segments.

Because the function docstring explicitly says “Build a working directory path under ~/.fileglancer/jobs/.” and the rest of the code (e.g. comments around the repo cache) assumes a server-owned directory, the first option (anchoring everything under a process-home-rooted ~/.fileglancer/jobs) is more consistent and simpler. To keep backwards compatibility with existing stored work_dir paths that already look like ~user/.fileglancer/jobs/..., we should limit our change to newly-built paths and ensure they are deterministic; switching to a root-based layout is acceptable if consumers treat work_dir as opaque.

Implementation details:

• In fileglancer/apps/core.py, near _REPO_CACHE_BASE, define _JOBS_BASE = Path(os.path.expanduser("~/.fileglancer/jobs")).

• Update _build_work_dir to:

  • Keep sanitising app_name, entry_point_id, and optional job_name_prefix as it already does.
  • Derive a safe username component, e.g. safe_user = _sanitize_for_path(username) if username else "default".
  • Build user_jobs_root = (_JOBS_BASE / safe_user).resolve().
  • Build work_dir = (user_jobs_root / f"{prefix}{job_id}-{safe_app}-{safe_ep}").resolve().
  • Optionally, assert that work_dir is under user_jobs_root by checking if user_jobs_root not in work_dir.parents and user_jobs_root != work_dir: raise ValueError(...). Given our construction, this is mostly a sanity check against path injection via _sanitize_for_path changes.

• All other uses of work_dir (such as work_dir.mkdir(...) and repo_link.symlink_to(cached_repo_dir)) remain unchanged; they simply operate on a now-guaranteed-safe path.

• No changes are needed in server.py or to imports beyond adding _JOBS_BASE declaration.

This keeps functionality (per-job directories per app/entry point) while ensuring username cannot redirect work directories to arbitrary locations.

General approach: Ensure that any user-influenced data used in filesystem paths is validated or sanitized so it cannot escape the intended directory structure. In this case, that means constraining username before using it in _build_work_dir, and ensuring that the resulting home directory is a real, safe user home and not a crafted path.

Best concrete fix without changing existing functionality:

1. Introduce a small helper like _sanitize_username in fileglancer/apps/core.py that enforces a conservative pattern for usernames (e.g., alphanumeric plus -_.), rejects dangerous characters (/, \, whitespace, .., NUL, etc.), and raises ValueError if invalid.
2. In _build_work_dir, call _sanitize_username(username) (when username is not None) before passing it to os.path.expanduser(f"~{username}"). This ensures that we only ever expand a well-formed username and cannot accidentally interpret a malicious path fragment as part of the tilde expression.
3. Optionally, resolve the resulting home path with Path(home).expanduser().resolve() and fail early if that resolution behaves unexpectedly; however, given the constraints and the existing behavior, just sanitizing username is sufficient and minimally invasive.
4. Because submit_job already propagates username into _build_work_dir, we do not need to change call sites—only the implementation of _build_work_dir and the new helper. Raising ValueError on invalid usernames is consistent with other validation patterns in this module (e.g., _parse_github_url uses ValueError), and submit_job already translates ValueError into HTTP 400.

Concretely:

• In fileglancer/apps/core.py, add _sanitize_username near the other helpers (_sanitize_for_path, _parse_github_url area).
• Update _build_work_dir so that if a username is given, we first sanitize it and then call os.path.expanduser(f"~{safe_username}"). The rest of _build_work_dir remains unchanged.

No new imports are required; we can use re which is already imported at the top of the file.

---

In general, to fix uncontrolled data in path expressions, we should either (1) avoid embedding untrusted data into path components used for filesystem operations, or (2) strictly validate and normalize any such data before constructing paths. For usernames, the typical approach is to restrict them to a safe character set and length and to handle lookup failures or invalid inputs explicitly.

For this specific case, the only problematic use of username is in _build_work_dir, where it is interpolated into ~{username}. We can fix this by introducing a small helper _sanitize_username_for_home(username: str) -> str that enforces a safe pattern and/or safely resolves the user’s home directory using pwd.getpwnam when running on Unix-like systems. However, we are instructed not to add new imports except well-known ones and not to assume much about the environment. A minimal, non-breaking, and portable fix is:

• Add a helper _sanitize_username_for_home near _build_work_dir that:
  • Rejects usernames containing /, \, whitespace, or NUL, and those starting with ~ or containing ...
  • Optionally constrains to a simple regex like ^[A-Za-z0-9._-]+$.
  • Raises ValueError if the username fails validation.
• Use this helper inside _build_work_dir before calling expanduser:
  • If username is provided, sanitize it and then compute home = os.path.expanduser(f"~{safe_username}").
  • If not, fall back to the current behavior with os.path.expanduser("~").
• Keep the rest of the function unchanged so that work_dir structure and behavior remain the same for valid usernames.

This confines the risk to a well-defined, validated subset of usernames and ensures that maliciously crafted values (such as ones containing path separators, additional tildes, or traversal sequences) are rejected early with a clear ValueError. The calling code (submit_job) already translates ValueError into a 400 HTTP response in the API handler, so we don’t need further changes to error handling.

Concretely:

• File fileglancer/apps/core.py:
  • Add _sanitize_username_for_home above _build_work_dir.
  • Modify _build_work_dir line 804 to sanitize username before using it, and to call expanduser with the sanitized value.

No changes are needed in fileglancer/server.py for this specific issue.

---

In general, to fix uncontrolled data in a path expression, we must ensure that untrusted input cannot cause the resulting path to escape a controlled root or perform unintended expansion. Here, the only tainted part is the username used with os.path.expanduser in _build_work_dir. We should (a) avoid using shell-like ~username expansion on untrusted data, and (b) restrict username to a safe form (e.g., a simple account name without path separators, dots, or null bytes). A robust approach is to resolve the user’s home directory using the system user database (pwd.getpwnam) after validating the username format; this prevents path traversal via a malicious "username" and ensures the path corresponds to a real account.

Concretely, we will adjust _build_work_dir in fileglancer/apps/core.py:

1. Add imports for pwd and re is already imported, so no new import for regex is needed.
2. Introduce a helper function _get_user_home(username: str) -> str that:
   • Validates username against a conservative regex like r"^[A-Za-z0-9_.-]+$" to disallow slashes and whitespace and reject empty or obviously malicious names.
   • Optionally rejects names containing ".." or NUL for extra safety.
   • Uses pwd.getpwnam(username).pw_dir to get the home directory, raising a clear ValueError if the user does not exist or if the name is invalid.
3. Modify _build_work_dir so that:
   • If username is provided, it calls _get_user_home(username) instead of os.path.expanduser(f"~{username}").
   • If username is None, it continues to use os.path.expanduser("~") as before.
   • The rest of the path logic (.fileglancer/jobs/...) stays unchanged.

This keeps all existing functionality (per-user job directories under each user’s real home) while ensuring username cannot be abused to craft arbitrary home paths. Since repo_link is derived from work_dir, this change ensures the symlink and directory operations are rooted under a verified, legitimate home directory rather than a path influenced by arbitrary expansion.

---

In general, to fix this type of issue you ensure that any user-derived data used in path construction is validated or constrained so it cannot cause access outside intended locations. For usernames specifically, you can restrict the allowed character set and/or ensure that the resolved path is under a known safe root after os.path.expanduser.

For this code, the minimal and robust fix is to sanitize/validate the username before using it in _build_work_dir. A straightforward approach is:

1. Add a small helper _sanitize_username_for_home in fileglancer/apps/core.py that:

   • Ensures username is non-empty.
   • Ensures it matches a conservative pattern, e.g. ^[A-Za-z0-9_.-]+$.
   • Optionally resolves ~username with os.path.expanduser and verifies the resulting path is an absolute directory under some expected base, but the character whitelist alone already prevents pathological values like ../../etc/passwd or embedded NULs.

2. Update _build_work_dir to call this sanitizer whenever a username was provided, and use the sanitized value in os.path.expanduser(f"~{username}").

This keeps the observable behavior identical for normal usernames (which already only use safe characters), but ensures that if any untrusted/invalid username ever reached this point, it will raise a clear ValueError instead of being used in a path. It also makes the data flow obviously safe to CodeQL because the username is explicitly validated before being interpolated into a filesystem path.

Concretely:

• In fileglancer/apps/core.py, define _sanitize_username_for_home near _build_work_dir.
• In _build_work_dir, before computing home, if username is not None, pass it through _sanitize_username_for_home and use the sanitized result in the expanduser call.
• No changes are needed in fileglancer/server.py.

---

[krokicki]

I would put this outside the "cluster" settings because there may be other paths we need to add which are not related to the cluster. Also, this is referenced in server.py in a place that shouldn't know about the cluster.

[krokicki]

I think this won't be necessary when using . /misc/lsf/conf/profile.lsf, but if it is, it should also go outside of the cluster config.