Harden service token configuration#73
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR externalizes service API tokens to environment variables, updates EventManager and Yeaster to require and validate those tokens, modifies environment configs to read them from getenv(), adds a repository validation script to detect hardcoded tokens and missing env var references, and documents the changes in setup.md. ChangesService Token Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
staff/web/build/p-5e3b44c4.entry.js (1)
1-1: Consider excluding build artifacts from version control.This minified build artifact makes detailed code review challenging. Modern best practices suggest:
- Add
staff/web/build/to.gitignore- Generate build artifacts during CI/CD deployment
- Only commit source files to version control
Benefits include: easier code review, reduced repository size, fewer merge conflicts, and clearer change history. This would be a project-wide architectural change beyond the scope of this security PR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff/web/build/p-5e3b44c4.entry.js` at line 1, The committed minified build artifact (exported symbol xero_profit and class r with componentWillLoad/render) should not be tracked; remove the file from version control, add the build output directory to .gitignore (exclude staff/web/build/), and update CI to produce build artifacts during deployment instead of committing them; ensure you also commit the removal (git rm --cached) so the repository only contains source files and not the generated class/function artifacts.docs/setup.md (1)
34-48: 💤 Low valueConsider documenting fallback behavior and token acquisition.
The new section clearly lists the required environment variables and their purposes. Consider adding:
- What happens when these variables are not set (e.g., does the application fail to start, or do features gracefully degrade?)
- How operators should obtain or generate these tokens initially
- Whether these are required in all environments or only production
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/setup.md` around lines 34 - 48, Update the "Service Integration Secrets" doc to explicitly state fallback behavior and token acquisition: for each env var (WALLET_API_KEY, YEASTER_MICROSERVICE_API_KEY, EVENT_MANAGER_ENDPOINT_API_KEY) add a short note on what happens if it's unset (fail fast vs graceful degradation and which features are disabled), where operators can obtain or generate the token (e.g., link to auth service, steps to create API key in provider, or local dev token generation), and which environments require them (production only vs required in dev/test). Also mention the sqsEndpoint/EventManager integration behavior specifically when EVENT_MANAGER_ENDPOINT_API_KEY is missing and any startup checks the app performs.scripts/check-service-token-hardening.py (2)
49-57: ⚡ Quick winEnvironment variable reference check may have false positives.
The simple substring search on line 53 will match the environment variable name even if it appears in comments, string literals, or documentation rather than actual
getenv()calls.♻️ More precise pattern
for env_name, base in REQUIRED_ENV_REFERENCES: found = False + pattern = re.compile(rf"getenv\s*\(\s*['\"{env_name}['\"]") for path in base.rglob("*.php"): text = path.read_text(encoding="utf-8", errors="ignore") - if env_name in text: + if pattern.search(text): found = True break if not found: failures.append(f"missing environment reference: {env_name}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-service-token-hardening.py` around lines 49 - 57, The current check iterates REQUIRED_ENV_REFERENCES and searches for env_name as a raw substring in PHP files, which yields false positives from comments/strings; update the logic to read the file and run a regex search for actual runtime access patterns (e.g., getenv\(\s*['"]ENV_NAME['"]\s*\), env\(\s*['"]ENV_NAME['"]\s*\), \$_ENV\[['"]ENV_NAME['"]\], \$_SERVER\[['"]ENV_NAME['"]\], and similar helpers) instead of a plain substring; modify the loop that uses env_name, base.rglob and path.read_text to build and test these patterns (escaping env_name) and only mark found=true when a regex match for an access pattern is found.
40-43: ⚡ Quick winConsider adding double-quote support to the walletManager literal check.
The current regex only detects hardcoded
apiKeyvalues enclosed in single quotes. If someone uses double quotes ("apiKey" => "value"), the check would not catch it.♻️ Proposed enhancement
wallet_literal = re.compile( - r"'walletManager'\s*=>\s*\[[\s\S]*?'apiKey'\s*=>\s*'(?!\s*\))", + r"['\"]walletManager['\"]\s*=>\s*\[[\s\S]*?['\"]apiKey['\"]\s*=>\s*['\"](?!\s*\))", re.MULTILINE, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-service-token-hardening.py` around lines 40 - 43, The wallet_literal regex (wallet_literal in scripts/check-service-token-hardening.py) only matches single-quoted keys/values; update it to accept either single or double quotes for the 'walletManager' key and the 'apiKey' value (use a quote-capturing group like (['"]) and backreference \1 so both "walletManager" => "..." and 'walletManager' => '...' are detected), and ensure the pattern still allows multiline matching as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/components/EventManager.php`:
- Around line 414-415: The code unconditionally builds an Authorization header
using $this->sqsEndpointApiKey which can be empty; add a guard in the
EventManager class before sending (e.g., in the method that constructs the
headers or the send/forward method) that checks if
trim($this->sqsEndpointApiKey) is empty and fail fast by throwing a clear
exception or logging an error and aborting the send instead of constructing
'Authorization' => 'Bearer '. Ensure the guard references
$this->sqsEndpointApiKey and the Authorization header construction so the header
is never sent when the token is missing.
In `@common/components/Yeaster.php`:
- Line 15: The microserviceApiKey property on class Yeaster can be null and
currently leads to a blank "Bearer " header when building requests; add a
validation in the component initialization (e.g., in Yeaster::__construct or an
init() method) that asserts $this->microserviceApiKey is non-empty and throw a
clear exception or log+exit if missing, and update the request-building sites
that use the property (the methods that construct the Authorization header) to
guard: only add "Authorization: Bearer {microserviceApiKey}" when the key is
present to avoid emitting "Bearer " when unset.
In `@environments/krushn/common/config/main-local.php`:
- Line 13: The config currently reads environment variables with getenv() which
can return false and lead to empty/invalid auth headers; for keys like
'microserviceApiKey', 'apiKey', and 'sqsEndpointApiKey' in the config array,
replace the direct getenv(...) usage with an explicit fallback expression (e.g.
use the ?: null pattern) so missing env vars become null instead of false/empty
string; update the occurrences around the 'microserviceApiKey' entry and the
other two entries mentioned to use this fallback.
---
Nitpick comments:
In `@docs/setup.md`:
- Around line 34-48: Update the "Service Integration Secrets" doc to explicitly
state fallback behavior and token acquisition: for each env var (WALLET_API_KEY,
YEASTER_MICROSERVICE_API_KEY, EVENT_MANAGER_ENDPOINT_API_KEY) add a short note
on what happens if it's unset (fail fast vs graceful degradation and which
features are disabled), where operators can obtain or generate the token (e.g.,
link to auth service, steps to create API key in provider, or local dev token
generation), and which environments require them (production only vs required in
dev/test). Also mention the sqsEndpoint/EventManager integration behavior
specifically when EVENT_MANAGER_ENDPOINT_API_KEY is missing and any startup
checks the app performs.
In `@scripts/check-service-token-hardening.py`:
- Around line 49-57: The current check iterates REQUIRED_ENV_REFERENCES and
searches for env_name as a raw substring in PHP files, which yields false
positives from comments/strings; update the logic to read the file and run a
regex search for actual runtime access patterns (e.g.,
getenv\(\s*['"]ENV_NAME['"]\s*\), env\(\s*['"]ENV_NAME['"]\s*\),
\$_ENV\[['"]ENV_NAME['"]\], \$_SERVER\[['"]ENV_NAME['"]\], and similar helpers)
instead of a plain substring; modify the loop that uses env_name, base.rglob and
path.read_text to build and test these patterns (escaping env_name) and only
mark found=true when a regex match for an access pattern is found.
- Around line 40-43: The wallet_literal regex (wallet_literal in
scripts/check-service-token-hardening.py) only matches single-quoted
keys/values; update it to accept either single or double quotes for the
'walletManager' key and the 'apiKey' value (use a quote-capturing group like
(['"]) and backreference \1 so both "walletManager" => "..." and 'walletManager'
=> '...' are detected), and ensure the pattern still allows multiline matching
as before.
In `@staff/web/build/p-5e3b44c4.entry.js`:
- Line 1: The committed minified build artifact (exported symbol xero_profit and
class r with componentWillLoad/render) should not be tracked; remove the file
from version control, add the build output directory to .gitignore (exclude
staff/web/build/), and update CI to produce build artifacts during deployment
instead of committing them; ensure you also commit the removal (git rm --cached)
so the repository only contains source files and not the generated
class/function artifacts.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d238ad75-35ec-4848-af3a-24022e02758c
📒 Files selected for processing (17)
common/components/EventManager.phpcommon/components/Yeaster.phpdocs/setup.mdenvironments/circle-ci/common/config/main-local.phpenvironments/dev-server-nginx-debug/common/config/main-local.phpenvironments/dev-server-nginx/common/config/main-local.phpenvironments/dev-server-railway/common/config/main-local.phpenvironments/dev-server/common/config/main-local.phpenvironments/dev/common/config/main-local.phpenvironments/docker/common/config/main-local.phpenvironments/krushn-nginx/common/config/main-local.phpenvironments/krushn/common/config/main-local.phpenvironments/prod-nginx/common/config/main-local.phpenvironments/prod-railway/common/config/main-local.phpenvironments/prod/common/config/main-local.phpscripts/check-service-token-hardening.pystaff/web/build/p-5e3b44c4.entry.js
|
Updated the demo video asset at the existing link to a 46-second dynamic verification recording. It now shows the changed scope, validation command output, and current green PR checks without exposing any secrets. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Follow-up on CodeRabbit feedback: commit 037e49e addresses the valid security-path items by adding fail-fast guards before Yeaster/EventManager can emit blank Bearer headers, normalizing missing env vars to null across environment configs, tightening the env-reference checker to runtime access patterns, adding double-quote coverage, and expanding setup docs with missing-token behavior. I intentionally did not remove the tracked staff build output directory as a broad repository policy change; this PR only removes the checked-in bearer-token usage from that artifact. |
|
Follow-up commit Validation rerun locally:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Follow-up validation now that PHP is available in my local environment:
All passed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Small demo-asset maintenance update: I re-encoded the existing demo video at the same release URL to H.264 for more reliable GitHub/browser playback. No code changes were made. Current asset digest: |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
staff/web/build/p-5e3b44c4.entry.js (1)
1-1: 💤 Low valueConsider removing the test component entirely.
The
xero_profitcomponent is now disabled and serves no functional purpose. If this was purely a test component, consider removing it completely rather than shipping disabled code in the production bundle. This would:
- Reduce bundle size
- Eliminate maintenance overhead
- Prevent confusion about the component's purpose
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@staff/web/build/p-5e3b44c4.entry.js` at line 1, The file exports a disabled test component (class r exported as xero_profit) that only renders "Xero test disabled" and sets profitData in componentWillLoad; remove the unused component and its export to avoid shipping dead code: delete the class definition, its style assignment (r.style) and the export { r as xero_profit }, and remove any imports solely used by this file (r, h, H from p-a94aef08.js) or references to xero_profit elsewhere; if any other modules reference xero_profit, either remove those references or replace them with the intended production component before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-service-token-hardening.py`:
- Around line 42-50: The current check only looks for a literal
walletManager.apiKey via the wallet_literal regex; add equivalent regexes (e.g.,
yeaster_literal and event_literal) that mirror wallet_literal but target the
keys "yeaster" -> "microserviceApiKey" and "eventManager" ->
"sqsEndpointApiKey", then in the same loop that reads each main-local.php use
those regexes to append failures (like f"{path.relative_to(ROOT)}:
yeaster.microserviceApiKey is not env-backed" and f"{path.relative_to(ROOT)}:
eventManager.sqsEndpointApiKey is not env-backed") when matches are found;
ensure you reuse the re.MULTILINE flag and the same negative lookahead pattern
(avoid literal values) so the new checks behave identically to wallet_literal.
---
Nitpick comments:
In `@staff/web/build/p-5e3b44c4.entry.js`:
- Line 1: The file exports a disabled test component (class r exported as
xero_profit) that only renders "Xero test disabled" and sets profitData in
componentWillLoad; remove the unused component and its export to avoid shipping
dead code: delete the class definition, its style assignment (r.style) and the
export { r as xero_profit }, and remove any imports solely used by this file (r,
h, H from p-a94aef08.js) or references to xero_profit elsewhere; if any other
modules reference xero_profit, either remove those references or replace them
with the intended production component before committing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 786a73c6-9fa3-4761-ad3d-32f4fa2cf9ce
📒 Files selected for processing (17)
common/components/EventManager.phpcommon/components/Yeaster.phpdocs/setup.mdenvironments/circle-ci/common/config/main-local.phpenvironments/dev-server-nginx-debug/common/config/main-local.phpenvironments/dev-server-nginx/common/config/main-local.phpenvironments/dev-server-railway/common/config/main-local.phpenvironments/dev-server/common/config/main-local.phpenvironments/dev/common/config/main-local.phpenvironments/docker/common/config/main-local.phpenvironments/krushn-nginx/common/config/main-local.phpenvironments/krushn/common/config/main-local.phpenvironments/prod-nginx/common/config/main-local.phpenvironments/prod-railway/common/config/main-local.phpenvironments/prod/common/config/main-local.phpscripts/check-service-token-hardening.pystaff/web/build/p-5e3b44c4.entry.js
| wallet_literal = re.compile( | ||
| r"['\"]walletManager['\"]\s*=>\s*\[[\s\S]*?['\"]apiKey['\"]\s*=>\s*['\"](?!\s*\))", | ||
| re.MULTILINE, | ||
| ) | ||
| for path in (ROOT / "environments").rglob("main-local.php"): | ||
| text = path.read_text(encoding="utf-8", errors="ignore") | ||
| if wallet_literal.search(text): | ||
| failures.append(f"{path.relative_to(ROOT)}: walletManager apiKey is not env-backed") | ||
|
|
There was a problem hiding this comment.
Add literal-value regression checks for Yeaster and EventManager keys.
The script enforces non-literal walletManager.apiKey, but it does not enforce equivalent checks for yeaster.microserviceApiKey and eventManager.sqsEndpointApiKey. A hardcoded regression for those keys can currently pass this gate.
Suggested patch
- wallet_literal = re.compile(
- r"['\"]walletManager['\"]\s*=>\s*\[[\s\S]*?['\"]apiKey['\"]\s*=>\s*['\"](?!\s*\))",
- re.MULTILINE,
- )
+ literal_value_checks = [
+ (
+ "walletManager apiKey is not env-backed",
+ re.compile(
+ r"['\"]walletManager['\"]\s*=>\s*\[[\s\S]*?['\"]apiKey['\"]\s*=>\s*['\"]",
+ re.MULTILINE,
+ ),
+ ),
+ (
+ "yeaster microserviceApiKey is not env-backed",
+ re.compile(
+ r"['\"]yeaster['\"]\s*=>\s*\[[\s\S]*?['\"]microserviceApiKey['\"]\s*=>\s*['\"]",
+ re.MULTILINE,
+ ),
+ ),
+ (
+ "eventManager sqsEndpointApiKey is not env-backed",
+ re.compile(
+ r"['\"]eventManager['\"]\s*=>\s*\[[\s\S]*?['\"]sqsEndpointApiKey['\"]\s*=>\s*['\"]",
+ re.MULTILINE,
+ ),
+ ),
+ ]
for path in (ROOT / "environments").rglob("main-local.php"):
text = path.read_text(encoding="utf-8", errors="ignore")
- if wallet_literal.search(text):
- failures.append(f"{path.relative_to(ROOT)}: walletManager apiKey is not env-backed")
+ for message, pattern in literal_value_checks:
+ if pattern.search(text):
+ failures.append(f"{path.relative_to(ROOT)}: {message}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-service-token-hardening.py` around lines 42 - 50, The current
check only looks for a literal walletManager.apiKey via the wallet_literal
regex; add equivalent regexes (e.g., yeaster_literal and event_literal) that
mirror wallet_literal but target the keys "yeaster" -> "microserviceApiKey" and
"eventManager" -> "sqsEndpointApiKey", then in the same loop that reads each
main-local.php use those regexes to append failures (like
f"{path.relative_to(ROOT)}: yeaster.microserviceApiKey is not env-backed" and
f"{path.relative_to(ROOT)}: eventManager.sqsEndpointApiKey is not env-backed")
when matches are found; ensure you reuse the re.MULTILINE flag and the same
negative lookahead pattern (avoid literal values) so the new checks behave
identically to wallet_literal.
3 participants
/claim #55
Contributes to #55
Scope
This is a bounded code-only service-token hardening slice that does not overlap with the already-open S3, SQS, MediaConvert, SES mailer, Xero client-secret, Cloudinary, Civil ID, or bucket-guardrail submissions.
Bearerheaders when Yeaster or EventManager endpoint tokens are missing.nullinstead of leakingfalseinto auth configuration.docs/setup.md.scripts/check-service-token-hardening.pyso service-token and browser-bearer regressions are caught before review.Safety Boundary
No live AWS/IAM, Xero, wallet, Yeaster, candidate data, account IDs, private records, or production services were accessed. This PR only changes checked-in code/config references and does not include secret values.
Demo
Privacy-safe demo video: https://github.com/digzrow-coder/studenthub/releases/download/service-token-hardening-demo-20260515/service-token-hardening-demo.mp4
Verification
Summary by CodeRabbit
Release Notes
Chores
Documentation
Bug Fixes