fix for local openid keypair and allow time ago in words for markdown#2732
Open
fix for local openid keypair and allow time ago in words for markdown#2732
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes introduce environment variable exports for worker count configuration in server startup scripts and implement file-backed caching for OpenID RSA keypairs with optional cross-process synchronization using lock files. Additionally, a new time formatting function is added to the frontend service layer. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spiffworkflow-backend/src/spiffworkflow_backend/config/openid/rsa_keys.py`:
- Around line 33-36: _read_key_pair_from_files currently treats any existing
files as valid which can accept truncated PEMs if writers update in-place; fix
the writers (the PEM-writing code at the other two occurrences referenced) to
write to a temporary file first, validate the PEM content (e.g., by attempting
to load/parse the private/public key) and then atomically replace the final path
with Path.replace(tmp_path, final_path). Also update _read_key_pair_from_files
(and the other read sites) to validate the PEMs after reading (e.g., attempt to
parse them) and return None if parsing fails so startup won't accept truncated
files.
- Around line 122-125: The current logic silently ignores a partial
configuration of OPENID_PRIVATE_KEY / OPENID_PUBLIC_KEY by returning
cached/generated keys; update the check around private_key and public_key (the
variables populated from OPENID_PRIVATE_KEY and OPENID_PUBLIC_KEY) so that if
exactly one is present you raise an explicit error (e.g.,
ValueError/RuntimeError) describing which key is missing instead of falling back
to _load_or_create_file_backed_keys(), otherwise if both are present return the
pair as before; ensure the exception message clearly names the missing env var
to aid debugging.
In `@spiffworkflow-frontend/src/services/FormattingService.tsx`:
- Around line 4-7: The formatter mapping spiffFormatFunctions defines
convert_seconds_to_time_ago_for_display currently pointing directly at
TimeAgo.inWords, but checkForSpiffFormats will call formatters with a string
argument while TimeAgo.inWords expects a number; update the spiffFormatFunctions
entry for convert_seconds_to_time_ago_for_display to wrap TimeAgo.inWords in a
small adapter that coerces the incoming string to a numeric value (e.g.,
Number(arg) or parseFloat(arg)) before calling TimeAgo.inWords so the adapter
accepts the string passed by checkForSpiffFormats and forwards a number to
TimeAgo.inWords.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2f3b983-c8bd-47fe-bb86-af7c1d805dc5
📒 Files selected for processing (4)
spiffworkflow-backend/bin/boot_server_in_dockerspiffworkflow-backend/bin/run_server_locallyspiffworkflow-backend/src/spiffworkflow_backend/config/openid/rsa_keys.pyspiffworkflow-frontend/src/services/FormattingService.tsx
Comment on lines
+33
to
+36
| 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 |
Contributor
There was a problem hiding this comment.
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 and replace() them, or validate the PEMs before returning them.
Also applies to: 53-56, 69-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spiffworkflow-backend/src/spiffworkflow_backend/config/openid/rsa_keys.py`
around lines 33 - 36, _read_key_pair_from_files currently treats any existing
files as valid which can accept truncated PEMs if writers update in-place; fix
the writers (the PEM-writing code at the other two occurrences referenced) to
write to a temporary file first, validate the PEM content (e.g., by attempting
to load/parse the private/public key) and then atomically replace the final path
with Path.replace(tmp_path, final_path). Also update _read_key_pair_from_files
(and the other read sites) to validate the PEMs after reading (e.g., attempt to
parse them) and return None if parsing fails so startup won't accept truncated
files.
Comment on lines
+122
to
+125
| if private_key and public_key: | ||
| return (private_key, public_key) | ||
|
|
||
| return private_key, public_key | ||
| return _load_or_create_file_backed_keys() |
Contributor
There was a problem hiding this comment.
Reject partial OpenID key configuration.
If exactly one of OPENID_PRIVATE_KEY / OPENID_PUBLIC_KEY is present, this silently ignores it and falls back to the cached/generated pair. That hides config mistakes and can pin a different keypair than the deployment intended.
🛠️ 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
Verify each finding against the current code and only fix it if needed.
In `@spiffworkflow-backend/src/spiffworkflow_backend/config/openid/rsa_keys.py`
around lines 122 - 125, The current logic silently ignores a partial
configuration of OPENID_PRIVATE_KEY / OPENID_PUBLIC_KEY by returning
cached/generated keys; update the check around private_key and public_key (the
variables populated from OPENID_PRIVATE_KEY and OPENID_PUBLIC_KEY) so that if
exactly one is present you raise an explicit error (e.g.,
ValueError/RuntimeError) describing which key is missing instead of falling back
to _load_or_create_file_backed_keys(), otherwise if both are present return the
pair as before; ensure the exception message clearly names the missing env var
to aid debugging.
Comment on lines
4
to
7
| 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: |
Contributor
There was a problem hiding this comment.
🧩 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 convert_seconds_to_time_ago_for_display formatter has a type mismatch: checkForSpiffFormats dispatches all formatters with a string argument, but TimeAgo.inWords expects a number. While this formatter is not currently used through SPIFF_FORMAT, the mismatch should be fixed to prevent latent bugs if the formatter is used in the future. Wrap the function with explicit coercion as proposed.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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: | |
| const spiffFormatFunctions: Record<string, (value: string) => string> = { | |
| convert_seconds_to_date_time_for_display: DateAndTimeService.formatDateTime, | |
| 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, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spiffworkflow-frontend/src/services/FormattingService.tsx` around lines 4 -
7, The formatter mapping spiffFormatFunctions defines
convert_seconds_to_time_ago_for_display currently pointing directly at
TimeAgo.inWords, but checkForSpiffFormats will call formatters with a string
argument while TimeAgo.inWords expects a number; update the spiffFormatFunctions
entry for convert_seconds_to_time_ago_for_display to wrap TimeAgo.inWords in a
small adapter that coerces the incoming string to a numeric value (e.g.,
Number(arg) or parseFloat(arg)) before calling TimeAgo.inWords so the adapter
accepts the string passed by checkForSpiffFormats and forwards a number to
TimeAgo.inWords.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.