Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if not next_url or urlsplit(next_url).netloc != '': | ||
| next_url = url_for('index.index', cid=user.ctx_case) | ||
|
|
||
| return redirect(next_url) No newline at end of file |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 months ago
To fix the problem, we need to enhance the validation of the next_url parameter to ensure it is safe for redirection. We can achieve this by:
- Improving the
is_safe_urlfunction to handle additional edge cases, such as URLs with backslashes and mistyped URLs. - Ensuring that the
next_urlis either a relative URL or has the same host as the current request.
We will modify the is_safe_url function to include these additional checks and update the _filter_next_url function to use the improved validation.
| @@ -81,7 +81,8 @@ | ||
| Check whether the target URL is safe for redirection by ensuring that it is either a relative URL or | ||
| has the same host as the current request. | ||
| has the same host as the current request. Also handles backslashes and mistyped URLs. | ||
| """ | ||
| target = target.replace('\\', '') | ||
| ref_url = urlparse(request.host_url) | ||
| test_url = urlparse(urljoin(request.host_url, target)) | ||
| return test_url.scheme in ('http', 'https') and ref_url.netloc == test_url.netloc | ||
| return test_url.scheme in ('http', 'https') and ref_url.netloc == test_url.netloc and not test_url.path.startswith('//') | ||
|
|
||
| @@ -94,4 +95,2 @@ | ||
| return url_for('index.index', cid=context_case) | ||
| # Remove backslashes to mitigate obfuscation | ||
| next_url = next_url.replace('\\', '') | ||
| if is_safe_url(next_url): |
WalkthroughThis pull request introduces extensive project improvements across every layer. The version was bumped in the configuration, and new files were added for containerization, Docker Compose, Kubernetes deployments, and CI/CD pipelines. E2E testing support and documentation (including code style and architecture) were enhanced. Meanwhile, a broad refactoring of the Flask back‑end occurred: routes and blueprints (both traditional and REST‑v2) were renamed, reorganized, and deprecated or removed where appropriate. In addition, several business logic modules have been updated for consistent naming and improved error handling, and new Socket.IO event handlers were added for real‑time interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as REST API (v2)
participant BL as Business Logic
participant DB as Database
C->>API: GET /api/v2/cases/assets?case_identifier=X
API->>BL: Retrieve asset details (assets_get_detailed)
BL->>DB: Query asset record for case X
DB-->>BL: Asset data returned
BL->>API: Format results via schema
API-->>C: 200 OK with asset data
sequenceDiagram
participant U as User
participant S as Socket.IO Server
U->>S: Emit "join-case-obj-notif" with channel info
S->>S: join_room(channel)
S-->>U: Emit confirmation notification (user joined)
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
source/app/business/auth.py (4)
17-29: Consider annotating the return type.Currently, this function returns either a user object or
None, but the signature does not reflect this. Adding an explicit return type (e.g.,-> Optional[User]) can improve code clarity and maintainability.-def _retrieve_user_by_username(username:str): +def _retrieve_user_by_username(username: str) -> Optional[User]: ...
30-56: Align the docstring with the actual return type.The docstring states it returns a “User object,” but it actually returns a serialized dictionary from
UserSchema. Consider updating the docstring for accuracy. Also, to enhance debugging, you may uselog.exception(e)instead oflog.error(e.__str__())to capture the stack trace.:return: User object if successful, None otherwise + #:return: Dictionary if successful, None otherwise - log.error(e.__str__()) + log.exception("Failed to validate LDAP login:")
58-77: Keep “validate” functions pure by removing session-altering side effects.Unlike
validate_ldap_login, this function callswrap_login_user, causing a side effect that creates a session. For consistency and improved separation of concerns, either centralize session handling in the caller or unify both validation flows to maintain a single approach.
109-110: Simplify nested conditions.Ruff suggests combining nested checks into a single
if. This can make the code more concise and reduce indentation.-if app.config['SERVER_SETTINGS']['enforce_mfa'] is True and is_oidc is False: - if "mfa_verified" not in session or session["mfa_verified"] is False: +if ( + app.config['SERVER_SETTINGS']['enforce_mfa'] + and not is_oidc + and ( + "mfa_verified" not in session + or session["mfa_verified"] is False + ) ): return redirect(url_for('mfa_verify'))🧰 Tools
🪛 Ruff (0.8.2)
109-110: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/app/business/auth.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
source/app/business/auth.py
109-110: Use a single if statement instead of nested if statements
(SIM102)
🪛 GitHub Check: CodeQL
source/app/business/auth.py
[warning] 134-134: URL redirection from remote source
Untrusted URL redirection depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: End to end tests
🔇 Additional comments (3)
source/app/business/auth.py (3)
79-87: Verify edge cases under proxies or subdomains.While this function helps prevent open redirects by matching the netloc, it might block legitimate subdomain redirects if your deployment uses them or if requests come through proxies with rewritten host headers. Verify that this meets your architectural needs.
88-100: Looks good for preventing open redirects.Your
_filter_next_urlfunction effectively ensures that untrusted or malformed URLs are discarded in favor of your site’s index page. This is a robust pattern for handling next URLs.
134-134: ** Existing concern about untrusted URL redirection.**GitHub advanced security flagged this line for untrusted redirection. However,
_filter_next_urlchecks domain safety and defaults to the index page. This mitigates open redirect risks unless subdomains or special proxy headers require broader matching.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 134-134: URL redirection from remote source
Untrusted URL redirection depends on a user-provided value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
3-3: Add Alternate Text to the Logo ImageThe updated image tag on this line no longer includes an alt attribute, which is required for accessibility and to satisfy markdownlint (MD045). Please include descriptive alternate text (e.g., "IRIS Logo") to improve accessibility.
- <img src="ui/public/assets/img/logo.ico" /> + <img src="ui/public/assets/img/logo.ico" alt="IRIS Logo" />🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Images should have alternate text (alt text)
null(MD045, no-alt-text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)source/app/blueprints/graphql/graphql_route.py(2 hunks)source/app/blueprints/rest/alerts_routes.py(25 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/app/blueprints/graphql/graphql_route.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
3-3: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: End to end tests
- GitHub Check: Generate graphQL documentation
- GitHub Check: Test API
- GitHub Check: Build docker app
🔇 Additional comments (8)
README.md (1)
9-9: Ensure Version Consistency in DocumentationThe “Current Version” displayed is still set to v2.4.20, which appears inconsistent with the beta release identified as v2.5.0-beta.1. For clarity and to prevent confusion among users, please update all version references accordingly (e.g., the version text and any related git checkout instructions).
- <i>Current Version v2.4.20</i> + <i>Current Version v2.5.0-beta.1</i>source/app/blueprints/rest/alerts_routes.py (7)
66-66: Blueprint renaming for REST API v2 transition.The blueprint name change from
alerts_blueprinttoalerts_rest_blueprintaligns with the PR objective of transitioning to REST API v2. This is part of the broader backend restructuring mentioned in the PR summary.
69-71: API versioning and deprecation strategy implemented.Good implementation of API versioning. The
endpoint_deprecateddecorator properly marks old endpoints while pointing to their new v2 counterparts.
159-159: Enhanced security with user context in database queries.The addition of
current_user_idparameter to theget_filtered_alertsfunction is a good security practice as it ensures database queries are scoped to the current user's permissions.
169-178: Improved error handling for schema field selection.The addition of try-except block with logging for schema field selection provides better error handling. If invalid fields are specified, the code gracefully falls back to using the full schema rather than failing.
835-835: More robust identifier parsing.Using
parse_comma_separated_identifiers()provides a more robust way to handle multiple identifiers compared to the previous manual string splitting approach.Also applies to: 904-904
416-420: Code clarity improvement in activity tracking.The restructured activity tracking and history entry code makes the purpose clearer by separating the tracking activity from adding the history entry.
217-218:Details
❓ Verification inconclusive
Note the change in validation behavior.
The addition of
partial=Trueparameter to schema loading changes validation behavior. This allows partial objects to be validated without requiring all fields, which makes the API more flexible but could potentially allow incomplete data if not handled properly.
🏁 Script executed:
#!/bin/bash # Look for other places where these schemas are used with partial=True to ensure consistent behavior rg "ioc_schema.load|asset_schema.load" -A 3 -B 3Length of output: 5434
Attention: Validate Impact of Partial Validation Change
The new use of
partial=Truein loadingiocsandassetsallows the API to accept partial data. This change is intentional and aligns with similar patterns elsewhere in the codebase (e.g., insource/app/business/iocs.py). However, please verify that downstream processing correctly handles incomplete data, ensuring that missing fields are either safely optional or explicitly managed.
- Confirm that alert processing (including deserialization in
alerts_routes.py) is robust against partial data.- Review downstream dependencies and tests to ensure that accepting incomplete objects does not introduce issues.
4 participants
This pull request introduces several significant changes to the backend, development environment setup, continuous integration workflow. The most important changes include the migration partial migration of the API to a REST API v2.
A restructuring of the backend code into business is also ongoing. The old API should still works but bugs are to be expected hence the beta tag. Bug and security patches are also included.
This is a BETA version and should not be put in production
API
Development Environment Setup:
Dockerfile,devcontainer.json, anddocker-compose.yml, to streamline the development environment setup.Continuous Integration Workflow:
Configuration Updates:
.env.modelto support additional features and services.Summary by CodeRabbit
New Features
/api/v2) for managing cases, assets, IOCs, tasks, dashboards, search, and user profiles—enabling enhanced data export and filtering.Improvements