fix: replace bare except clauses and add path validation in PAM base

[jlima8900]

Summary

• Replace all 35 bare except: with specific exception types
• Add path traversal check in file validation methods

Test plan

• Tested on Python 3.7 and 3.12

[aaunario-keeper]

Let's break down each call site. All of them follow the same pattern:

try: data = json.loads(data) if isinstance(data, str) else data
except (json.JSONDecodeError, ValueError, TypeError, KeyError): logging.error(...)

The try block is doing exactly one thing: conditionally calling json.loads(data). So here's what each exception type actually means in that specific context:

json.JSONDecodeError — The string was valid Python but not valid JSON (malformed braces, bad escaping, trailing commas, etc.). This is the most expected and meaningful one to catch here. The log message should say something like "invalid JSON syntax".

ValueError — Since JSONDecodeError is already a subclass of ValueError, a plain ValueError from json.loads would only be raised in Python <3.5. In Python 3.5+ it's always JSONDecodeError. So this adds nothing in practice and can be dropped unless there's a specific Python 2/old-Python compat concern.

TypeError — json.loads raises TypeError if the argument is not a string, bytes, or bytearray. But the code already guards this with isinstance(data, str) — if it's not a string, json.loads is never called. So TypeError can never be raised here. It's dead code in every single call site.

KeyError — json.loads never raises KeyError. This could only arise from dict access. In these try blocks, no dict access is happening. So KeyError is also dead code at every call site.

---

The import_schedule_field case is different:

try:
    parsed_cron = vault.TypedField.import_schedule_field(obj.cron)
except (json.JSONDecodeError, ValueError, TypeError, KeyError):
    parsed_cron = {}

This one wraps a library call rather than a raw json.loads, so ValueError, TypeError, and KeyError are more plausible depending on what import_schedule_field does internally. But the catch-all style here still obscures what went wrong. Ideally you'd want separate handlers with separate log messages.

---

Making it more granular and helpful

For all the json.loads call sites, the exception list should really just be json.JSONDecodeError. Then the log messages can be made more specific:

# Instead of:
except (json.JSONDecodeError, ValueError, TypeError, KeyError):
    logging.error(f"PAM Scripts failed to load from: {str(data)[:80]}...")

# Do this:
except json.JSONDecodeError as e:
    logging.error(
        f"PAM Scripts: invalid JSON at line {e.lineno}, col {e.colno} — {e.msg}. "
        f"Input (truncated): {str(data)[:80]}"
    )

json.JSONDecodeError carries .msg, .lineno, .colno, and .pos — all of which are genuinely useful for diagnosing what went wrong in a malformed config without having to reproduce the issue.

For the import_schedule_field site:

try:
    parsed_cron = vault.TypedField.import_schedule_field(obj.cron)
except json.JSONDecodeError as e:
    logging.error(f"CRON field contains invalid JSON: {e.msg} at pos {e.pos}. Value: {obj.cron!r}")
    parsed_cron = {}
except (ValueError, KeyError) as e:
    logging.error(f"CRON field has unexpected structure ({type(e).__name__}): {e}. Value: {obj.cron!r}")
    parsed_cron = {}

This way you can tell at a glance from the log whether the CRON string couldn't even be parsed as JSON vs. whether it parsed fine but was structurally wrong.

---

Summary of recommended changes:

Call site	Drop	Keep	Improve
All json.loads sites	ValueError, TypeError, KeyError	json.JSONDecodeError	Add e.msg, e.lineno, e.colno to log
import_schedule_field	Flatten into one tuple	Split into separate except blocks	Distinguish parse error vs. structural error

The net effect is fewer caught exceptions (safer), more actionable log messages (more debuggable), and no dead TypeError/KeyError catches cluttering the exception lists.

[jlima8900]

Great catch — you're absolutely right on all points.

Dropping ValueError, TypeError, KeyError from the json.loads sites makes sense. JSONDecodeError is a ValueError subclass so it's redundant, and TypeError/KeyError are unreachable given the isinstance(data, str) guard.

Richer log messages using e.msg, e.lineno, e.colno — agreed, that's much more debuggable than just truncating the input.

import_schedule_field split — makes sense to separate JSON parse errors from structural errors since it wraps a library call with different failure modes.

I'll update the PR with these changes:

• Narrow all json.loads sites to json.JSONDecodeError only
• Add structured error details (e.msg, e.lineno, e.colno) to log messages
• Split the import_schedule_field handler into separate JSONDecodeError vs (ValueError, KeyError) blocks