Potential fix for code scanning alert no. 141: Full server-side request forgery#8093
Open
grantfitzsimmons wants to merge 1 commit into
Open
Potential fix for code scanning alert no. 141: Full server-side request forgery#8093grantfitzsimmons wants to merge 1 commit into
grantfitzsimmons wants to merge 1 commit into
Conversation
…st forgery Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses code scanning alert #141 (full SSRF) by restricting which remote URLs can be fetched when loading default tree mapping JSON, introducing a server-side allowlist and rejecting other remote sources before issuing requests.get.
Changes:
- Added
_is_allowed_remote_mapping_url()to allow onlyhttps://files.specifysoftware.organd specific mapping-style paths. - Enforced the allowlist in
load_default_tree_json()by rejecting disallowed sources prior torequests.get.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def _is_allowed_remote_mapping_url(source: str) -> bool: | ||
| parsed = urlparse(source.strip()) | ||
| if parsed.scheme != 'https' or parsed.netloc != 'files.specifysoftware.org': |
Comment on lines
+48
to
+49
| if parsed.path.startswith('/treerows/'): | ||
| stem = Path(unquote(parsed.path)).stem.lower() |
Comment on lines
132
to
133
| response = requests.get(source) | ||
| response.raise_for_status() |
Comment on lines
+129
to
+130
| if not _is_allowed_remote_mapping_url(source): | ||
| raise ValueError('Remote mapping URL is not allowed.') |
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
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.
Potential fix for https://github.com/specify/specify7/security/code-scanning/141
General fix: enforce a server-side allowlist for any remotely fetched mapping URL, and reject all other remote URLs before calling
requests.get. Keep existing local-path behavior unchanged.Best fix in this code: in
specifyweb/backend/trees/default_tree_files.py, add a validation helper that only permits HTTPS URLs tofiles.specifysoftware.organd only for known mapping paths under/treerows/that are already handled by local mapping logic (KNOWN_REMOTE_DEFAULT_TREE_PATHSplus the existing/treerows/*.jsondiscipline mapping pattern). Then call this validator inload_default_tree_jsonright beforerequests.get(source). If invalid, raiseValueErrorso existing callers’except Exceptionpaths return their current 404 error response without broader functional changes.This single change covers both alert variants because both flows call
load_default_tree_json(mapping_url).Suggested fixes powered by Copilot Autofix. Review carefully before merging.