Fix proactive AWS credential refresh silently skipping all credentials - #580
Fix proactive AWS credential refresh silently skipping all credentials#5805Devanshu wants to merge 1 commit into
Conversation
Cache keys are {uid}:{role_arn}:{external_id}:{policy_hash}, but the
refresh task recovered the ARN via split(':')[0], which returns the
user ID since ARNs contain colons. This caused proactive refresh to
silently skip every credential. Reconstruct the key prefix and match
with startswith() instead. Adds regression tests.
WalkthroughModified ChangesAWS Credential Refresh Prefix Matching
Estimated code review effort: 3 (Moderate) | ~20 minutes GitHub Skill MCP Tools Documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Sequence Diagram(s)sequenceDiagram
participant Task as refresh_aws_credentials
participant Cache as _credential_cache
participant DB as Database
participant STS as assume_workspace_role
Task->>Cache: read expiring cache keys within refresh window
Task->>DB: query active AWS connections
DB-->>Task: return user_id, role_arn, external_id rows
loop for each connection row
Task->>Task: build cache_key_prefix from user_id, role_arn, external_id
Task->>Cache: check if any expiring key starts with prefix
alt prefix matches
Task->>STS: assume_workspace_role(role_arn, external_id, workspace_id, region, user_id)
STS-->>Task: refreshed credentials
else no match
Task->>Task: skip row
end
end
Task-->>Task: return refreshed and skipped counts
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![arvo-ai-staging[bot]](https://avatars.githubusercontent.com/in/4088260?s=60&v=4){kind=small}
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/chat/backend/agent/skills/integrations/github/SKILL.md`:
- Around line 49-50: Add a blank line after the “### MCP Tools” heading in
SKILL.md so the list starts separated from the heading and satisfies
markdownlint MD022. Update the markdown around the “MCP Tools” section by
inserting a single empty line before the “- Files:” list item, keeping the rest
of the section unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: dcbb2dd2-cba5-417b-90fb-4ea760592a5f
📒 Files selected for processing (3)
server/chat/backend/agent/skills/integrations/github/SKILL.mdserver/tests/utils/test_credential_refresh.pyserver/utils/aws/credential_refresh.py
| ### MCP Tools (for direct GitHub API operations beyond RCA) | ||
| - Files: `get_file_contents`, `create_or_update_file`, `push_files`, `get_repository_tree` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line after the heading.
markdownlint-cli2 flags MD022 here: the ### MCP Tools heading is immediately followed by the list item, so it is not surrounded by blank lines. Insert one blank line after Line 49 to keep the docs check clean.
♻️ Proposed fix
### MCP Tools (for direct GitHub API operations beyond RCA)
+
- Files: `get_file_contents`, `create_or_update_file`, `push_files`, `get_repository_tree`📝 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.
| ### MCP Tools (for direct GitHub API operations beyond RCA) | |
| - Files: `get_file_contents`, `create_or_update_file`, `push_files`, `get_repository_tree` | |
| ### MCP Tools (for direct GitHub API operations beyond RCA) | |
| - Files: `get_file_contents`, `create_or_update_file`, `push_files`, `get_repository_tree` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 `@server/chat/backend/agent/skills/integrations/github/SKILL.md` around lines
49 - 50, Add a blank line after the “### MCP Tools” heading in SKILL.md so the
list starts separated from the heading and satisfies markdownlint MD022. Update
the markdown around the “MCP Tools” section by inserting a single empty line
before the “- Files:” list item, keeping the rest of the section unchanged.
Source: Linters/SAST tools
1 participant
Problem
The proactive STS credential refresh task in
server/utils/aws/credential_refresh.pynever refreshes any credentials.
Cache keys are built in
aws_sts_client.pyas:The refresh task tried to recover the role ARN from each key with
k.split(":")[0]. Because AWS ARNs themselves contain colons(
arn:aws:iam::123456789012:role/MyRole),split(":")[0]returns theuid, not the ARN. The later checkif role_arn not in expiring_role_arnsthen compares a real ARN against a set of user IDs, which never matches —
so every credential is skipped and the task silently does nothing. No error
is raised, so the failure is invisible in normal operation.
Fix
Instead of parsing the ARN back out of the key, reconstruct the deterministic
prefix from the DB row and match with
startswith():The trailing
:anchors the match to the field boundary, so a role such as.../role/Admincannot accidentally match.../role/AdminReadOnly. Becausepolicy_hashis the final field, the prefix correctly matches all cachedpolicy variants for the same connection.
Testing
Adds
server/tests/utils/test_credential_refresh.pywith two tests:test_expiring_connection_is_refreshed— a near-expiry cache entry triggersa re-assume. Fails on current
main, passes with this fix.test_prefix_does_not_match_longer_role— proves theAdminprefix does notmatch
AdminReadOnly.Both pass locally.
Scope
This fixes the matching only. The task re-assumes the full-policy variant, so
restricted (
session_policy) cache entries are still not proactively refreshed— that's pre-existing behavior and out of scope here, but could be a follow-up.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests