fix(calendar): redact script diagnostics, and page the calendar list

Three findings from CodeRabbit, all valid.

Raw subprocess output was being returned to the client -- the script's
stderr on one path, and its own error payload on another. CodeQL flagged
the same line. That script handles OAuth client secrets and interpolates
exceptions into its messages, so either could carry a secret or a path.
Both now go to the log unredacted, where they are worth having in full,
and reach the client through a redactor.

That redactor already existed inside describe_exception, which only
takes exceptions. Split out as redact_text: an exception is not the only
thing worth returning, and a subprocess's stderr is just as capable of
quoting a token.

calendarList.list returns 100 entries per page by default, caps at 250,
and hands back a nextPageToken when there are more. Reading one page
would have hidden calendars from the picker with nothing to say the list
was cut short. It now pages, asking for 250 at a time, bounded at ten
pages so a malformed token cannot spin.

And a test helper was a lambda where ruff wants a def.

The five new tests fail against the previous commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
This commit is contained in:
ChuckBuilds
2026-08-13 09:34:26 -04:00
co-authored by Claude Opus 5
parent 1b9ecc0f19
commit 838f0b9d8a
3 changed files with 168 additions and 10 deletions
+19
View File
@@ -77,6 +77,25 @@ def describe_exception(exc: BaseException,
""" """
message = str(exc).strip() message = str(exc).strip()
text = f"{type(exc).__name__}: {message}" if message else type(exc).__name__ text = f"{type(exc).__name__}: {message}" if message else type(exc).__name__
return redact_text(text, max_length)
def redact_text(text: str, max_length: int = _MAX_DETAIL_LENGTH) -> str:
"""Make arbitrary text safe to hand back over HTTP.
Split out of describe_exception because exceptions are not the only thing
worth returning: a subprocess's stderr, or a message a helper script
printed, is just as useful to a user and just as capable of carrying a
token or a password in it.
Args:
text: The text to redact
max_length: Truncate beyond this many characters
Returns:
A single line, credentials replaced, length capped.
"""
text = text or ''
# Order matters: the URL and header forms are more specific than the # Order matters: the URL and header forms are more specific than the
# generic key=value pattern, which would otherwise chew the scheme. # generic key=value pattern, which would otherwise chew the scheme.
text = _REDACT_URL_USERINFO.sub(r'\1<redacted>\3', text) text = _REDACT_URL_USERINFO.sub(r'\1<redacted>\3', text)
@@ -189,10 +189,21 @@ class TestListingShape:
import types import types
fake_pickle = types.SimpleNamespace(load=lambda f: creds, dump=lambda *a: None) fake_pickle = types.SimpleNamespace(load=lambda f: creds, dump=lambda *a: None)
fake_build = lambda *a, **k: types.SimpleNamespace( pages = items if isinstance(items, list) and items and isinstance(items[0], dict) \
calendarList=lambda: types.SimpleNamespace( else items
list=lambda: types.SimpleNamespace( if isinstance(pages, list):
execute=lambda: {'items': items}))) pages = [{'items': pages}]
state = {'i': 0}
def fake_list(**kwargs):
page = pages[min(state['i'], len(pages) - 1)]
state['i'] += 1
return types.SimpleNamespace(execute=lambda: page)
def fake_build(*args, **kwargs):
return types.SimpleNamespace(
calendarList=lambda: types.SimpleNamespace(list=fake_list))
real_import = __builtins__['__import__'] if isinstance(__builtins__, dict) \ real_import = __builtins__['__import__'] if isinstance(__builtins__, dict) \
else __builtins__.__import__ else __builtins__.__import__
@@ -239,3 +250,95 @@ class TestListingShape:
self._authenticate(client, monkeypatch, [{'summary': 'ghost'}, {'id': 'real@x'}]) self._authenticate(client, monkeypatch, [{'summary': 'ghost'}, {'id': 'real@x'}])
body = client.get('/api/v3/plugins/calendar/list-calendars').get_json() body = client.get('/api/v3/plugins/calendar/list-calendars').get_json()
assert [c['id'] for c in body['calendars']] == ['real@x'] assert [c['id'] for c in body['calendars']] == ['real@x']
class TestPagination:
"""calendarList.list pages at 250 and defaults to 100."""
def _paged(self, client, monkeypatch, pages):
import types
creds = type('C', (), {'expired': False, 'refresh_token': None, 'valid': True})()
(client.plugin_dir / 'token.pickle').write_bytes(b'x')
state = {'i': 0}
seen = []
def fake_list(**kwargs):
seen.append(kwargs)
page = pages[min(state['i'], len(pages) - 1)]
state['i'] += 1
return types.SimpleNamespace(execute=lambda: page)
def fake_build(*args, **kwargs):
return types.SimpleNamespace(
calendarList=lambda: types.SimpleNamespace(list=fake_list))
real_import = __builtins__['__import__'] if isinstance(__builtins__, dict) \
else __builtins__.__import__
def fake_import(name, *args, **kwargs):
if name == 'pickle':
return types.SimpleNamespace(load=lambda f: creds, dump=lambda *a: None)
if name == 'google.auth.transport.requests':
return types.SimpleNamespace(Request=object)
if name == 'googleapiclient.discovery':
return types.SimpleNamespace(build=fake_build)
return real_import(name, *args, **kwargs)
monkeypatch.setattr('builtins.__import__', fake_import)
return seen
def test_every_page_is_collected(self, client, monkeypatch):
# Taking only the first page would hide calendars from the picker with
# nothing to say the list was cut short.
self._paged(client, monkeypatch, [
{'items': [{'id': 'a@x', 'summary': 'A'}], 'nextPageToken': 't1'},
{'items': [{'id': 'b@x', 'summary': 'B'}], 'nextPageToken': 't2'},
{'items': [{'id': 'c@x', 'summary': 'C'}]},
])
body = client.get('/api/v3/plugins/calendar/list-calendars').get_json()
assert [c['id'] for c in body['calendars']] == ['a@x', 'b@x', 'c@x']
def test_the_page_token_is_passed_back(self, client, monkeypatch):
seen = self._paged(client, monkeypatch, [
{'items': [{'id': 'a@x', 'summary': 'A'}], 'nextPageToken': 'tok'},
{'items': [{'id': 'b@x', 'summary': 'B'}]},
])
client.get('/api/v3/plugins/calendar/list-calendars')
assert seen[0]['pageToken'] is None
assert seen[1]['pageToken'] == 'tok'
assert all(k['maxResults'] == 250 for k in seen)
def test_a_looping_token_cannot_spin_forever(self, client, monkeypatch):
# Every page claims another follows.
self._paged(client, monkeypatch, [
{'items': [{'id': 'a@x', 'summary': 'A'}], 'nextPageToken': 'same'},
])
body = client.get('/api/v3/plugins/calendar/list-calendars').get_json()
assert body['status'] == 'success'
assert len(body['calendars']) <= mod._CALENDAR_LIST_MAX_PAGES
class TestDiagnosticsAreRedacted:
def test_script_stderr_is_redacted_on_the_way_out(self, tmp_path):
script = tmp_path / 'calendar_registration.py'
script.write_text(
'import sys\n'
'sys.stderr.write("boom client_secret=hunter2 more\\n")\n',
encoding='utf-8')
payload, error = mod._run_calendar_registration(tmp_path, '')
assert payload is None
assert 'hunter2' not in error, error
assert '<redacted>' in error, error
def test_a_failing_script_payload_is_redacted(self, client):
(client.plugin_dir / 'credentials.json').write_text('{}', encoding='utf-8')
(client.plugin_dir / 'calendar_registration.py').write_text(
'import json\n'
'print(json.dumps({"status": "error", '
'"message": "Failed: client_secret=topsecret"}))\n',
encoding='utf-8')
body = client.post('/api/v3/plugins/calendar/authenticate',
json={}).get_json()
assert body['status'] == 'error'
assert 'topsecret' not in json.dumps(body), body
assert '<redacted>' in body['message'], body
+42 -6
View File
@@ -22,7 +22,7 @@ logger = logging.getLogger(__name__)
from src.web_interface.api_helpers import success_response, error_response, validate_request_json from src.web_interface.api_helpers import success_response, error_response, validate_request_json
from src.web_interface.errors import ErrorCode from src.web_interface.errors import ErrorCode
from src.web_interface.secret_helpers import find_secret_fields, separate_secrets from src.web_interface.secret_helpers import find_secret_fields, separate_secrets
from src.web_interface.error_handler import describe_exception from src.web_interface.error_handler import describe_exception, redact_text
from src.plugin_system.operation_types import OperationType from src.plugin_system.operation_types import OperationType
from src.web_interface.validators import ( from src.web_interface.validators import (
validate_file_upload validate_file_upload
@@ -7312,6 +7312,11 @@ def upload_calendar_credentials():
logger.error('Error in upload_calendar_credentials', exc_info=True) logger.error('Error in upload_calendar_credentials', exc_info=True)
return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details', 'details': describe_exception(e)}), 500 return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details', 'details': describe_exception(e)}), 500
# calendarList.list pages at 250 entries maximum. Ten pages is far past any
# real account and exists only so a malformed nextPageToken cannot spin here.
_CALENDAR_LIST_MAX_PAGES = 10
def _calendar_plugin_dir() -> Optional[Path]: def _calendar_plugin_dir() -> Optional[Path]:
"""Where the calendar plugin is installed, or None if it is not.""" """Where the calendar plugin is installed, or None if it is not."""
if api_v3.plugin_manager: if api_v3.plugin_manager:
@@ -7363,9 +7368,15 @@ def _run_calendar_registration(plugin_dir: Path, stdin_payload: str):
if isinstance(payload, dict): if isinstance(payload, dict):
return payload, None return payload, None
detail = (result.stderr or result.stdout or '').strip()[:300] raw = (result.stderr or result.stdout or '').strip()
# The unredacted text goes to the log, where it is worth having in full.
# What comes back over HTTP is redacted: this is a script that handles
# OAuth client secrets, and its stderr can quote them.
if raw:
logger.error('calendar_registration.py failed (exit %s): %s',
result.returncode, raw)
return None, 'Authentication script produced no result%s' % ( return None, 'Authentication script produced no result%s' % (
': %s' % detail if detail else '') ': %s' % redact_text(raw) if raw else '')
@api_v3.route('/plugins/calendar/authenticate', methods=['POST']) @api_v3.route('/plugins/calendar/authenticate', methods=['POST'])
@@ -7404,8 +7415,14 @@ def authenticate_calendar():
return jsonify({'status': 'error', 'message': error}), 500 return jsonify({'status': 'error', 'message': error}), 500
if payload.get('status') != 'success': if payload.get('status') != 'success':
# The script's own diagnosis is more useful than anything that # The script's own diagnosis is more useful than anything that
# could be reconstructed here. # could be reconstructed here -- but it interpolates exceptions
return jsonify(payload), 400 # into its messages, so it reaches the client redacted and the
# original goes to the log.
logger.error('calendar authentication failed: %s', payload)
safe = dict(payload)
safe['message'] = redact_text(str(payload.get('message', '')
or 'Authentication failed'))
return jsonify(safe), 400
return jsonify(payload) return jsonify(payload)
except Exception as e: except Exception as e:
@@ -7469,7 +7486,26 @@ def list_calendar_calendars():
}), 400 }), 400
service = build_google_service('calendar', 'v3', credentials=creds) service = build_google_service('calendar', 'v3', credentials=creds)
entries = service.calendarList().list().execute().get('items', [])
# calendarList.list returns 100 entries per page by default and caps at
# 250, handing back a nextPageToken when there are more. Taking only
# the first page would silently hide calendars from the picker, and the
# user would have no way to tell the list was truncated.
entries = []
page_token = None
for _ in range(_CALENDAR_LIST_MAX_PAGES):
response = service.calendarList().list(
maxResults=250, pageToken=page_token).execute()
entries.extend(response.get('items', []))
page_token = response.get('nextPageToken')
if not page_token:
break
else:
# 2500 calendars in, something is wrong with the account or the
# token is looping; show what was collected rather than spin.
logger.warning(
'calendarList paging stopped at %d pages with more remaining',
_CALENDAR_LIST_MAX_PAGES)
calendars = [{ calendars = [{
'id': entry.get('id'), 'id': entry.get('id'),