fix(session-flow): degrade a failing tzdata bundle to exit 3, not exit 1

[kyle-sexton]

Closes #2672

Summary

The tzdata bundling that fixed #2647 shipped two defects, both from that PR's final commit (e86c2a98), which changed vendoring from a checked-in directory to a runtime-extracted zip after the last security review — both bot reviews pin 1b514176, which never extracted anything.

The #2647 fix itself is sound and untouched here: IANA zones still resolve on a host with no system TZDB.

Fix

1. A failing bundle no longer reports "the limit still holds." _ensure_bundled_tzdata() had no exception handling around ZipFile/extractall, so a corrupt or truncated zip raised at import time and exited 1 — which in this script's contract means the limit is still active, hand back and stop. Every failure now degrades to exit 3, matching what a missing bundle already did.

2. The cache is no longer trusted blindly. The directory name now carries a per-user component, and a cache hit is trusted only when the directory is not a symlink and — where the platform can report ownership — is owned by this user.

On the ownership check's honest limits: os.getuid does not exist on Windows, so that half is a no-op there and the per-user directory name carries the isolation instead. That is stated in the docstring rather than implied away. It is also the right shape for the actual risk: %TEMP% is already per-user on Windows, so the predictable-shared-directory problem is a POSIX-shared-/tmp problem.

Verification

Degradation, fixed vs. origin/main — scratch copy of the script with a sabotaged bundle, asking a question whose answer should be "lifted":

case	before	after
corrupt zip	exit 1	exit 3
truncated zip	exit 1	exit 3
bundle path is a directory	exit 3	exit 3
missing zip	exit 3	exit 3

Planted-cache probe — computing the cache path from the committed zip's digest, planting tzdata/__init__.py, running the script:

before:  planted=session-flow-tzdata-40438214aeac6481          executed=True
after:   planted=session-flow-tzdata-40438214aeac6481          executed=False

Exit codes 0/1/2 unchanged for valid input:

resets 2:30am (America/New_York)  --now 2026-08-14T10:00  → 0  (lifted)
resets 11:30pm (America/New_York) --now 2026-08-14T22:00  → 1  (blocked)
no clause here                                            → 2  (unparsed)

Suite: 12/12 pass (7 pre-existing + 5 new) on Windows 11 / CPython 3.14. The corrupt and truncated cases fail against the pre-change code, so the tests prove the fix rather than merely accompanying it.

One test-design note: chmod 000 is not a portable probe for the unreadable-bundle class — it does not restrict an Administrator on Windows NTFS, so the file stays readable and the run legitimately succeeds. The suite uses a directory at the bundle path instead. I found this by writing the weaker test first and watching it pass for the wrong reason on both branches.

Not run: the wider plugin-gate suite. This plugin's Python tests run only on ubuntu-24.04 in CI, so the POSIX half of the ownership check — the branch that actually compares st_uid, and the only branch where the planted-directory scenario is a real cross-user vector — is exercised by CI and not by me. Everything above was verified on Windows.

Related note, deliberately not fixed

A typo'd zone (Not/AZone) now yields exit 3 with "report the timezone failure", where it previously yielded 2. Exit 3 is documented in keep-going/SKILL.md and the CHANGELOG so the change is disclosed, but genuinely unparsable input arguably still belongs at 2. Left for a maintainer decision.

Separately, and filed in #2672's closing note: the checker cannot parse a date-form reset clause — "resets Aug 18, 8pm (America/New_York)" returns unparsed (exit 2), because only same-day time forms are recognized. A weekly limit states its reset exactly that way, so the script is blind to the longest limits. I hit this live while recovering from a weekly limit during this work.

No version bump or CHANGELOG entry — a consolidating bump is landing separately, and parallel PRs staging the same version collide on check-changelog-parity.sh --check-order.

Related

N/A

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 2m 48s —— View job

---

Security review complete

• Gather context (PR diff, changed files, skip gate)
• Review _ensure_bundled_tzdata() cache/ownership logic for trust-boundary issues
• Review exception handling / exit-code degradation logic
• Review test file for security-relevant coverage
• Check for zizmor-covered findings to defer (not re-report)
• Post final findings

No security issues found in the files changed by this PR (check-usage-limit-reset.py, check-usage-limit-reset.test.py). No workflow files are touched, so there's nothing here for the Actions-trigger/permissions/supply-chain class of findings either.

This PR is itself a security hardening change (predictable-tempdir / planted-cache mitigation, CWE-377/CWE-426), so I focused on whether the new trust logic actually closes the gap it claims to:

• _cache_is_trusted() is called unconditionally on every path that reaches a tzdata directory at cache_root — whether that directory was just extracted, found pre-existing, or produced by a losing race on staging.rename(cache_root) (the OSError fallback at L82-87). I traced the case where an attacker pre-creates the digest-and-uid-named directory in a shared /tmp before the victim runs (the scenario the PR's "planted-cache probe" verifies): since a non-privileged attacker cannot chown to the victim's uid, the ownership check at L127-129 rejects it regardless of which code path reached it — the check is a true final gate at the point of use, not something a race between the existence check and the rename can bypass.
• The broad except Exception: return in _ensure_bundled_tzdata() only widens which failures degrade to exit 3; SystemExit/KeyboardInterrupt/GeneratorExit aren't Exception subclasses, so they still propagate normally — the handler doesn't accidentally suppress control-flow signals.
• The zip is extracted with ZipFile.extractall() unchanged from the pre-existing session-flow: check-usage-limit-reset.py exits 2 on Windows for any IANA timezone (zoneinfo has no tzdata) #2647 code (not new in this diff), and it's the maintainer-vendored, checked-in zip rather than attacker-controlled input, so a zip-slip concern there is pre-existing and out of scope for this review.
• The Windows path (os.getuid absent) is a documented, deliberate no-op — the PR text is upfront about it, and it's consistent with %TEMP% already being per-user there, so I don't think it needs a fix.

One thing I looked at but am not flagging as a finding: _cache_is_trusted() calls cache_root.is_symlink() and os.stat(cache_root) as two separate syscalls, which is a TOCTOU window in principle (CWE-367). I didn't file it because exploiting it requires an attacker to delete/replace a directory they don't own inside a sticky-bit temp dir, which POSIX sticky-bit semantics on /tmp already deny to a non-owning, non-root, non-directory-owning process — i.e., no concrete exploitable path under the standard /tmp permission model this fix already relies on elsewhere. Worth a second look only if this code ever runs against a shared temp directory without the sticky bit set.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a196c90fd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Last security-reviewed head: 4a196c90fdfb7c68c864ad088f6bfc7f6925000a. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

Claude finished @kyle-sexton's task in 5m 29s —— View job

---

Code review complete

• Gather context (PR body, diff, changed files)
• Read CLAUDE.md / REVIEW.md guidelines
• Review check-usage-limit-reset.py changes
• Review check-usage-limit-reset.test.py changes
• Post findings as inline comments
• Final summary

Scope note: This repo carries a claude-security-review.yml workflow, so per REVIEW.md this lane (code-review) stays out of security scope — the cache-planting/ownership hardening in _cache_is_trusted is left for the security lane. The findings below are correctness/test-coverage only.

Findings

🔴 Important — check-usage-limit-reset.test.py L127‑145: the three new BundledTzdataDegradationTests subprocess tests (corrupt/truncated/unreadable-bundle) run the script without clearing PYTHONTZPATH. The plugin-gate CI lane that runs check-usage-limit-reset.test.sh executes on ubuntu-24.04, which ships /usr/share/zoneinfo — zoneinfo.ZoneInfo resolves from system TZPATH before ever falling back to tzdata, so a sabotaged bundle is irrelevant there and the script will exit 0, not the 3 these tests assert. The codebase already has the fix for this exact trap: the pre-existing test_iana_timezone_parses_with_bundled_tzdata sets env={"PYTHONTZPATH": ""} specifically to hide the system zoneinfo tree. The PR's "12/12 pass" was verified only on Windows 11 (no system TZDB, so the gap is invisible there); the PR body explicitly discloses the ubuntu-24.04 plugin-gate run wasn't done. Left an inline comment with a suggested fix (env={**os.environ, "PYTHONTZPATH": ""}).

🟡 Nit — check-usage-limit-reset.test.py L158‑163: test_unreadable_bundle_degrades_to_timezone_unavailable hits the pre-existing _VENDOR_ZIP.is_file() guard rather than the new try/except around ZipFile/extractall, so it re-confirms unchanged #2647 behavior instead of covering the new fix, unlike its two siblings.

Everything else — the _ensure_bundled_tzdata exception handling degrading to exit 3, the exit-code contract, resolve_zone's error message — reads correctly and matches the documented intent. Nice catch on the chmod 000-isn't-portable-on-Windows point in the test docstring; the reasoning there checks out.
· branch fix/2648-tzdata-degradation

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @cursor[bot]'s task in 3m 1s —— View job

---

Code review complete

• Gather context (PR diff via gh pr diff, changed files, REVIEW.md scope split)
• Skip gate check (open, not draft, non-trivial, new commits since the prior code-review pass)
• Review check-usage-limit-reset.py changes (correctness/design/error-handling)
• Review check-usage-limit-reset.test.py changes (test coverage, incl. prior review follow-ups)
• Verify prior findings (PYTHONTZPATH, "unreadable" test naming) were actually addressed
• Post summary

Scope note: This repo carries claude-security-review.yml, so per REVIEW.md this lane stays out of security scope (_cache_is_trusted's ownership/symlink hardening) — that's the security lane's territory, already reviewed separately on this PR.

What changed since the last code-review pass

Two follow-up commits landed after the prior claude-review pass (head 4a196c90f) and this pass ran against the new head (1cab692):

• 6f19a0e adds env={**os.environ, "PYTHONTZPATH": ""} to the _run_with_bundle subprocess helper in check-usage-limit-reset.test.py L127‑150.
• 1cab692 expands the directory-at-bundle-path test's docstring to cite session-flow: check-usage-limit-reset.py exits 2 on Windows for any IANA timezone (zoneinfo has no tzdata) #2647/session-flow: a corrupt tzdata bundle exits 1 ("limit still holds") and a predictable temp cache is put on sys.path #2672 and clarify it exercises the pre-existing is_file() guard rather than the new try/except.

Both directly address the two findings raised on the previous head (mine and Codex's, both pointing at the same PYTHONTZPATH gap; and the nit about the "unreadable" test not covering the new exception path). I confirmed by diffing 4a196c90f..1cab692 — no other lines changed besides the version bump and CHANGELOG entry.

Findings

None. I re-read the full script and test file (not just the incremental diff) looking for anything the fixup commits might have introduced or missed, and didn't find anything a careful reviewer would block or flag:

• _run_with_bundle (test L127‑150) now hides the system zoneinfo tree exactly like the pre-existing test_iana_timezone_parses_with_bundled_tzdata, so the three subprocess tests actually exercise the sabotaged-bundle path on the ubuntu-24.04 plugin-gate runner instead of silently passing via /usr/share/zoneinfo.
• test_directory_at_bundle_path_degrades_to_timezone_unavailable (test L165‑173) is now named and documented for what it actually covers (the pre-existing is_file() guard, not the new exception handling), consistent with its two siblings that do cover the new try/except.
• The core _ensure_bundled_tzdata/_cache_is_trusted logic in check-usage-limit-reset.py is unchanged from the previously-reviewed head.

Verification note

I could not execute the test suite myself — this lane's tool grant is gh pr diff/view/comment/review + Read/Glob/Grep, no general command execution, so I wasn't able to run check-usage-limit-reset.test.py locally to independently confirm the fix (if that capability is wanted for this lane, it'd need adding to --allowedTools). This PR's own ci run's plugin-gate job (the one that runs this suite on ubuntu-24.04, the exact runner class the earlier findings were about) was still in progress as of this review — worth confirming it's green before merge, since that's the first real execution of these tests against a Linux system TZDB.
· branch fix/2648-tzdata-degradation