fix(ci): restore read-all at workflow level for Scorecard verification#327
fix(ci): restore read-all at workflow level for Scorecard verification#327imran-siddique merged 9 commits intomicrosoft:mainfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add EU AI Act, Colorado AI Act, and GPAI obligations timeline with AGT coverage mapping. Reference Microsoft Purview DSPM for AI as complementary data governance layer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Scorecard API rejects workflows with write permissions at the workflow level. id-token: write and security-events: write must be scoped to the job level only. Restores permissions: read-all at workflow level while keeping job-level write permissions intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request addresses a CI failure caused by the OpenSSF Scorecard's stricter requirements for workflow-level permissions. The proposed fix reverts the workflow-level permissions to read-all while keeping the necessary write permissions (security-events: write, id-token: write) scoped at the job level. This change aligns with the Scorecard's expectations and resolves the issue without compromising security.
The PR also includes unrelated documentation updates, which are not directly tied to the CI fix but provide additional context about the project's compliance with OWASP and regulatory standards.
Feedback
🔴 CRITICAL
No critical issues identified in this PR.
🟡 WARNING
- Unrelated Documentation Changes in CI Fix PR
- The PR includes documentation updates (e.g., OWASP compliance and regulatory alignment) that are unrelated to the CI fix. While these changes are valuable, they should ideally be submitted in a separate PR to maintain a clear scope and facilitate focused reviews.
- Action: Consider splitting the documentation changes into a separate PR to align with best practices for atomic commits.
💡 SUGGESTIONS
-
Add a Comment Explaining Workflow-Level Permissions
- While the PR description explains the rationale for reverting to
permissions: read-all, this context is not present in the workflow file itself. Future maintainers may not immediately understand why this change was necessary. - Action: Add a comment in the
scorecard.ymlfile explaining whypermissions: read-allis required at the workflow level for Scorecard verification.
# Workflow-level permissions must be read-only for OpenSSF Scorecard verification. # Write permissions are scoped to the job level below. permissions: read-all
- While the PR description explains the rationale for reverting to
-
Add a Test or Validation for Workflow Permissions
- To prevent future regressions, consider adding a validation step in the CI pipeline to ensure that workflow-level permissions remain compliant with Scorecard requirements.
- Action: Investigate whether a pre-commit hook or a custom GitHub Action can be used to validate workflow permissions before merging changes.
-
Document CI/CD Workflow Permissions in README
- The documentation does not currently explain the CI/CD workflow permissions strategy. Adding a brief section about the use of
read-allat the workflow level and scoped write permissions at the job level would improve transparency. - Action: Add a section to the README or a dedicated CI/CD documentation file explaining the permissions strategy.
- The documentation does not currently explain the CI/CD workflow permissions strategy. Adding a brief section about the use of
General Observations
- The fix is well-aligned with the OpenSSF Scorecard documentation and resolves the immediate issue without introducing new security risks.
- The documentation updates are well-written and provide valuable context for the project's compliance with OWASP and regulatory standards, but they should be decoupled from this PR for clarity.
Final Recommendation
- Merge this PR after addressing the suggestion to add a comment in the workflow file explaining the
permissions: read-allchange. - Consider creating a separate PR for the documentation updates to maintain a clear scope for each PR.
🤖 AI Agent: security-scannerSecurity Analysis of the Pull Request1. Prompt Injection Defense BypassNo changes in this PR directly affect prompt injection defenses. The modifications are limited to CI/CD workflow permissions and documentation updates. No issues found. 2. Policy Engine CircumventionThe change restores 3. Trust Chain WeaknessesThe PR does not introduce any changes related to SPIFFE/SVID validation, certificate pinning, or other trust chain mechanisms. No issues found. 4. Credential ExposureThe change to 5. Sandbox EscapeThis PR does not involve changes to sandboxing or container isolation mechanisms. No issues found. 6. Deserialization AttacksNo deserialization logic is introduced or modified in this PR. No issues found. 7. Race ConditionsThe PR does not introduce any concurrency-related changes or time-of-check-to-time-of-use (TOCTOU) vulnerabilities. No issues found. 8. Supply ChainThe PR does not introduce new dependencies or modify existing ones. The Scorecard action is already part of the workflow, and no changes to its version or source are made. No issues found. Rating of Findings
Suggested Fix
Final AssessmentThe changes in this PR are necessary to comply with OpenSSF Scorecard requirements and do not introduce significant security risks. The broader permission scope at the workflow level is mitigated by job-level scoping of write permissions. However, the slight increase in the attack surface warrants a LOW rating for potential risks. |
…ft#324) Add Google-style docstrings with Args, Returns, Raises, Attributes, and Example sections to MCPMessageType, MCPAdapter, and MCPServer classes. Also enhances docstrings for key methods including handle_message, _handle_tools_call, _handle_resources_read, and _map_tool_to_action. Fixes microsoft#316
Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
…s (dependency confusion) (microsoft#325) - Replace !pip install agent-os with !pip install -e ../.. in all 6 notebooks; agent-os is not on PyPI and installing it from PyPI is a dependency confusion vector - Replace zendesk-sdk/freshdesk-sdk with zenpy/freshdesk (the real published SDKs) in customer-service/requirements.txt - Remove hashlib-compat from healthcare-hipaa/requirements.txt; hashlib is stdlib and hashlib-compat is not a real PyPI package
…stall agent-os with agent-os-kernel Replace all remaining instances of `pip install agent-os` (unregistered on PyPI) with `pip install agent-os-kernel` (the actual package) across docs, examples, TypeScript extensions, CLI source, tests, and SVG assets. Also fixes `pip install emk` references to point to `agent-os-kernel[full]` since emk is a submodule, not a standalone PyPI package. Completes the fix started in PR microsoft#325 which only covered notebooks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 AI Agent: test-generator🧪 Test Coverage Analysis
|
🤖 AI Agent: docs-sync-checker📝 Documentation Sync ReportIssues Found
Suggestions
Additional Notes
Action Items
Once these issues are addressed, the documentation will be in sync. |
🤖 AI Agent: breaking-change-detector🔍 API Compatibility ReportSummaryThe recent changes in the Findings
Migration GuideTo migrate from the old package name to the new one, users should replace any instances of For example:
Ensure that all references in code, documentation, and CI/CD workflows are updated accordingly. |
🤖 AI Agent: security-scannerSecurity Analysis of the Pull Request1. Prompt Injection Defense Bypass
2. Policy Engine Circumvention
3. Trust Chain Weaknesses
4. Credential Exposure
5. Sandbox Escape
6. Deserialization Attacks
7. Race Conditions
8. Supply Chain Risks
Summary of Findings
Final Recommendation
|
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request addresses a critical issue with the OpenSSF Scorecard CI workflow by restoring permissions: read-all at the workflow level while maintaining scoped write permissions at the job level. Additionally, it includes several documentation updates, dependency corrections, and a significant renaming of the agent-os package to agent-os-kernel. The PR also introduces a new GitHub Actions workflow for link checking and updates the MCP adapter with improved documentation and functionality.
Below is a detailed review of the changes:
🔴 CRITICAL
-
Potential Security Issue: Missing Governance Check for
_handle_prompts_list- File:
mcp_adapter.py - Issue: The
_handle_prompts_listmethod does not appear to include any governance checks before returning the list of prompts. This could lead to unauthorized access to sensitive prompt templates. - Recommendation: Add a governance check to ensure that the agent has the necessary permissions to access the list of prompts. For example:
if not self.control_plane.check_permission(self.agent_context, ActionType.PROMPTS_LIST): raise PermissionError("Access to list prompts is denied by governance policy.")
- File:
-
Potential Sandbox Escape in
_handle_resources_read- File:
mcp_adapter.py - Issue: The
_handle_resources_readmethod does not sanitize or validate theuriparameter before attempting to read the resource. This could allow an attacker to craft a maliciousurithat accesses unauthorized files or resources. - Recommendation: Implement strict validation for the
uriparameter, ensuring that it adheres to expected formats and does not allow directory traversal or access to unintended resources.
- File:
🟡 WARNING
-
Breaking Change: Package Renaming
- Issue: The
agent-ospackage has been renamed toagent-os-kernel. This is a breaking change for any users or systems that depend on the previous package name. - Recommendation: Clearly document this change in the release notes and provide a migration guide for users. Consider adding a deprecation warning in the old package (if it still exists) to inform users of the change.
- Issue: The
-
Backward Compatibility for MCP Adapter
- File:
mcp_adapter.py - Issue: The updated MCP adapter introduces new arguments (
tool_mapping,on_block,logger) to its constructor. This could break existing code that instantiatesMCPAdapterwithout these arguments. - Recommendation: Ensure backward compatibility by providing default values for the new arguments. For example:
def __init__(self, control_plane, agent_context, mcp_handler=None, tool_mapping=None, on_block=None, logger=None): ...
- File:
💡 SUGGESTIONS
-
Improved Documentation for MCP Adapter
- File:
mcp_adapter.py - Observation: The updated docstrings for
MCPAdapterand its methods are clear and detailed. However, consider adding a high-level example of how the adapter integrates with the rest of the system (e.g., how it interacts with the control plane and the agent context).
- File:
-
Testing for MCP Governance Checks
- File:
mcp_adapter.py - Observation: The governance checks in methods like
_handle_tools_calland_handle_resources_readare critical for security. However, there is no evidence in the PR that these checks are thoroughly tested. - Recommendation: Add unit tests to verify that governance checks are correctly enforced for all MCP methods, including edge cases (e.g., unknown tools, invalid URIs, etc.).
- File:
-
Link Checker Workflow
- File:
.github/workflows/link-check.yml - Observation: The new link-check workflow is a good addition for maintaining documentation quality. However, it is limited to Markdown files (
**/*.md). - Recommendation: Consider extending the link checker to other file types that may contain links, such as HTML or JSON files.
- File:
-
Dependency Updates
- File:
requirements.txtin various examples - Observation: The PR corrects some dependency issues (e.g., replacing
zendesk-sdkwithzenpyand removinghashlib-compat). However, it would be beneficial to run a dependency audit to ensure all dependencies are up-to-date and free of known vulnerabilities. - Recommendation: Use tools like
pip-auditorsafetyto perform a dependency audit and address any flagged issues.
- File:
-
Type Safety and Pydantic Validation
- File:
mcp_adapter.py - Observation: The MCP adapter methods rely on dictionary-based inputs (
Dict[str, Any]) without explicit validation. - Recommendation: Use Pydantic models to validate the structure and types of the incoming JSON-RPC messages. This will improve type safety and reduce the risk of runtime errors.
- File:
-
Thread Safety
- File:
mcp_adapter.py - Observation: The MCP adapter does not appear to use any thread synchronization mechanisms. If the adapter is used in a multi-threaded environment, there could be race conditions when accessing shared state (e.g.,
registered_toolsandregistered_resources). - Recommendation: Use thread-safe data structures (e.g.,
threading.Lockorconcurrent.futures.ThreadPoolExecutor) to ensure thread safety.
- File:
Final Assessment
- The PR addresses the immediate CI issue with OpenSSF Scorecard and introduces several improvements to the repository.
- However, there are critical security issues related to governance checks and potential sandbox escapes in the MCP adapter that must be addressed before merging.
- Additionally, the renaming of the
agent-ospackage is a breaking change and should be clearly communicated to users. - Several suggestions for improvement have been provided, including enhanced documentation, dependency audits, and type safety.
Recommendation: Address the critical and warning issues before merging. Consider implementing the suggested improvements to further enhance the quality and security of the codebase.
Fix OpenSSF Scorecard CI Failure
Root cause: The Scorecard API verification rejects workflows that have write permissions at the workflow level. Our recent security sweep (#303) changed
permissions: read-allto explicitsecurity-events: write+id-token: writeat the workflow level, triggering this failure.Error:
Fix: Restore
permissions: read-allat the workflow level. The write permissions (security-events: write,id-token: write) are already correctly scoped at the job level where Scorecard expects them.Reference: https://github.com/ossf/scorecard-action#workflow-restrictions