-
Notifications
You must be signed in to change notification settings - Fork 73
fix for local openid keypair and allow time ago in words for markdown #2732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,89 @@ | |
| # This should only be used for development and demonstration. SHOULD NOT BE USED IN PROD. | ||
|
|
||
| import os | ||
| import tempfile | ||
| from pathlib import Path | ||
|
|
||
|
|
||
| def _expected_worker_count() -> int: | ||
| configured_count = os.getenv("SERVER_WORKER_COUNT", "1") | ||
| try: | ||
| return int(configured_count) | ||
| except ValueError: | ||
| return 1 | ||
|
|
||
|
|
||
| def _key_cache_dir() -> Path: | ||
| configured_dir = os.getenv("OPENID_KEY_CACHE_DIR") | ||
| if configured_dir: | ||
| return Path(configured_dir) | ||
| return Path(tempfile.gettempdir()) / "spiffworkflow-dev-openid-keys" | ||
|
|
||
|
|
||
| def _key_paths() -> tuple[Path, Path, Path]: | ||
| key_dir = _key_cache_dir() | ||
| return ( | ||
| key_dir / "openid-private.pem", | ||
| key_dir / "openid-public.pem", | ||
| key_dir / ".lock", | ||
| ) | ||
|
|
||
|
|
||
| def _read_key_pair_from_files(private_key_path: Path, public_key_path: Path) -> tuple[str, str] | None: | ||
| if private_key_path.exists() and public_key_path.exists(): | ||
| return (private_key_path.read_text(), public_key_path.read_text()) | ||
| return None | ||
|
|
||
|
|
||
| def _load_or_create_file_backed_keys_with_lock() -> tuple[str, str]: | ||
| private_key_path, public_key_path, lock_path = _key_paths() | ||
| private_key_path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| with lock_path.open("w") as lock_file: | ||
| import fcntl | ||
|
|
||
| fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) | ||
|
|
||
| existing_keys = _read_key_pair_from_files(private_key_path, public_key_path) | ||
| if existing_keys is not None: | ||
| return existing_keys | ||
|
|
||
| private_key, public_key = _generate_keys() | ||
| private_key_path.write_text(private_key) | ||
| public_key_path.write_text(public_key) | ||
| os.chmod(private_key_path, 0o600) | ||
| os.chmod(public_key_path, 0o644) | ||
| return (private_key, public_key) | ||
|
|
||
|
|
||
| def _load_or_create_file_backed_keys_without_lock() -> tuple[str, str]: | ||
| private_key_path, public_key_path, _lock_path = _key_paths() | ||
| private_key_path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| existing_keys = _read_key_pair_from_files(private_key_path, public_key_path) | ||
| if existing_keys is not None: | ||
| return existing_keys | ||
|
|
||
| private_key, public_key = _generate_keys() | ||
| private_key_path.write_text(private_key) | ||
| public_key_path.write_text(public_key) | ||
| os.chmod(private_key_path, 0o600) | ||
| os.chmod(public_key_path, 0o644) | ||
| return (private_key, public_key) | ||
|
|
||
|
|
||
| def _load_or_create_file_backed_keys() -> tuple[str, str]: | ||
| try: | ||
| import fcntl # noqa: F401 | ||
| except ImportError as err: | ||
| if _expected_worker_count() > 1: | ||
| raise RuntimeError( | ||
| "Built-in OpenID dev keys require OPENID_PRIVATE_KEY and OPENID_PUBLIC_KEY " | ||
| "when running with multiple workers on platforms without fcntl-based file locking." | ||
| ) from err | ||
| return _load_or_create_file_backed_keys_without_lock() | ||
|
|
||
| return _load_or_create_file_backed_keys_with_lock() | ||
|
|
||
|
|
||
| def _generate_keys() -> tuple[str, str]: | ||
|
|
@@ -32,15 +115,14 @@ def _generate_keys() -> tuple[str, str]: | |
|
|
||
|
|
||
| def _initialize_keys() -> tuple[str, str]: | ||
| """Initialize keys from environment or generate them.""" | ||
| """Initialize keys from environment or load a shared cached keypair.""" | ||
| private_key = os.getenv("OPENID_PRIVATE_KEY") | ||
| public_key = os.getenv("OPENID_PUBLIC_KEY") | ||
|
|
||
| if not private_key or not public_key: | ||
| # Generate keys if not provided via environment | ||
| private_key, public_key = _generate_keys() | ||
| if private_key and public_key: | ||
| return (private_key, public_key) | ||
|
|
||
| return private_key, public_key | ||
| return _load_or_create_file_backed_keys() | ||
|
Comment on lines
+122
to
+125
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reject partial OpenID key configuration. If exactly one of 🛠️ Proposed fix def _initialize_keys() -> tuple[str, str]:
"""Initialize keys from environment or load a shared cached keypair."""
private_key = os.getenv("OPENID_PRIVATE_KEY")
public_key = os.getenv("OPENID_PUBLIC_KEY")
- if private_key and public_key:
+ if private_key is not None or public_key is not None:
+ if not private_key or not public_key:
+ raise RuntimeError(
+ "OPENID_PRIVATE_KEY and OPENID_PUBLIC_KEY must either both be set to non-empty values or both be unset."
+ )
return (private_key, public_key)
return _load_or_create_file_backed_keys()🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| class OpenIdConfigsForDevOnly: | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,9 @@ | ||||||||||||||||||||||||||||||||||||||||
| import DateAndTimeService from './DateAndTimeService'; | ||||||||||||||||||||||||||||||||||||||||
| import { TimeAgo } from '../helpers/timeago'; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const spiffFormatFunctions: { [key: string]: Function } = { | ||||||||||||||||||||||||||||||||||||||||
| convert_seconds_to_date_time_for_display: DateAndTimeService.formatDateTime, | ||||||||||||||||||||||||||||||||||||||||
| convert_seconds_to_time_ago_for_display: TimeAgo.inWords, | ||||||||||||||||||||||||||||||||||||||||
| convert_seconds_to_duration_for_display: | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
4
to
7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Verify the formatter dispatch passes string captures
rg -nP 'spiffFormatFunctions\[spiffFormat\]\(originalValue\)|const spiffFormatFunctions|convert_seconds_to_time_ago_for_display' \
spiffworkflow-frontend/src/services/FormattingService.tsx
# Verify TimeAgo.inWords contract assumes numeric input
rg -nP 'function inWords\s*\(timeAgo:\s*number\)|const milliseconds = timeAgo \* 1000' \
spiffworkflow-frontend/src/helpers/timeago.tsRepository: sartography/spiff-arena Length of output: 349 🏁 Script executed: #!/bin/bash
# Get full context of FormattingService.tsx around the formatter dispatch
cat -n spiffworkflow-frontend/src/services/FormattingService.tsx | head -40Repository: sartography/spiff-arena Length of output: 1373 🏁 Script executed: #!/bin/bash
# Check DateAndTimeService signatures for the other formatters
rg -n 'formatDateTime|formatDurationForDisplay|ymdDateStringToConfiguredFormat' \
spiffworkflow-frontend/src/services/DateAndTimeService.tsx | head -20Repository: sartography/spiff-arena Length of output: 328 🏁 Script executed: #!/bin/bash
# Get full function signatures for DateAndTimeService formatters
sed -n '99p;209p;233p' spiffworkflow-frontend/src/services/DateAndTimeService.tsxRepository: sartography/spiff-arena Length of output: 224 🏁 Script executed: #!/bin/bash
# Check if TimeAgo.inWords is called anywhere else directly
rg -n 'TimeAgo\.inWords|inWords' spiffworkflow-frontend/src --type tsx --type ts | grep -v 'spiffFormatFunctions\|import'Repository: sartography/spiff-arena Length of output: 94 🏁 Script executed: #!/bin/bash
# Check if TimeAgo.inWords is called anywhere else directly (without file type filter)
rg -n 'TimeAgo\.inWords|inWords' spiffworkflow-frontend/src | grep -v 'spiffFormatFunctions\|import' | head -20Repository: sartography/spiff-arena Length of output: 986 🏁 Script executed: #!/bin/bash
# Check the types in TaskTable.tsx to see what entry.created_at_in_seconds is
rg -B5 -A5 'entry\.created_at_in_seconds' spiffworkflow-frontend/src/components/TaskTable.tsx | head -20Repository: sartography/spiff-arena Length of output: 606 🏁 Script executed: #!/bin/bash
# Check what type entry.created_at_in_seconds is (looking for type definitions)
rg -n 'created_at_in_seconds' spiffworkflow-frontend/src --type-list -A2 -B2 | head -30Repository: sartography/spiff-arena Length of output: 456 🏁 Script executed: #!/bin/bash
# Search for type definitions or interfaces that include created_at_in_seconds
rg -n 'created_at_in_seconds' spiffworkflow-frontend/src -A1 -B1 | head -40Repository: sartography/spiff-arena Length of output: 2077 🏁 Script executed: #!/bin/bash
# Search for actual usage of SPIFF_FORMAT formatter to see if convert_seconds_to_time_ago_for_display is actually used
rg -n 'SPIFF_FORMAT.*convert_seconds_to_time_ago_for_display|convert_seconds_to_time_ago_for_display' spiffworkflow-frontendRepository: sartography/spiff-arena Length of output: 186 Add explicit numeric coercion for the new time-ago formatter. The Proposed fix-const spiffFormatFunctions: { [key: string]: Function } = {
+const spiffFormatFunctions: Record<string, (value: string) => string> = {
convert_seconds_to_date_time_for_display: DateAndTimeService.formatDateTime,
- convert_seconds_to_time_ago_for_display: TimeAgo.inWords,
+ convert_seconds_to_time_ago_for_display: (value: string) => {
+ const seconds = Number(value);
+ if (!Number.isFinite(seconds)) {
+ console.warn(`attempted: ${value}, but value is not a valid epoch-seconds number`);
+ return value;
+ }
+ return TimeAgo.inWords(seconds);
+ },
convert_seconds_to_duration_for_display:
DateAndTimeService.formatDurationForDisplay,
convert_date_to_date_for_display:
DateAndTimeService.ymdDateStringToConfiguredFormat,
};📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||
| DateAndTimeService.formatDurationForDisplay, | ||||||||||||||||||||||||||||||||||||||||
| convert_date_to_date_for_display: | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the cached PEM writes crash-safe.
_read_key_pair_from_files()treats any two existing files as valid, but both writers update the final PEM paths in place. If a write is interrupted, startup can accept truncated PEM data and the first OpenID request fails later. Please write temp files andreplace()them, or validate the PEMs before returning them.Also applies to: 53-56, 69-72
🤖 Prompt for AI Agents