test: add smoke tests for organizations handler endpoints#70
Open
SatishKumar620 wants to merge 8 commits intoOWASP-BLT:mainfrom
Open
test: add smoke tests for organizations handler endpoints#70SatishKumar620 wants to merge 8 commits intoOWASP-BLT:mainfrom
SatishKumar620 wants to merge 8 commits intoOWASP-BLT:mainfrom
Conversation
owasp-blt
bot
added
unresolved-conversations: 0
needs-peer-review
|
👋 Hi @SatishKumar620! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
WalkthroughRefactors organizations handlers to use ORM models, adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as OrganizationsHandler
participant ORM as ORM/Models
participant DB as Database
Client->>Handler: HTTP GET /organizations or /organizations/{id}?include=stats
Handler->>Handler: validate params (id, is_active, type ∈ ALLOWED_ORG_TYPES)
Handler->>ORM: build ORM query (filters, filter_or, pagination)
ORM->>DB: execute SQL
DB-->>ORM: rows / counts
ORM-->>Handler: objects and totals
alt include=stats or GET /organizations/{id}/stats
Handler->>Handler: call _get_organization_stats(org_id)
Handler->>ORM: aggregation queries (domain_count, bug_count, verified_bug_count, manager_count)
ORM->>DB: execute aggregations
DB-->>ORM: counts
ORM-->>Handler: stats dict
end
Handler-->>Client: HTTP response (list/detail with optional stats and paginated managers or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 (3)
tests/test_organizations.py (3)
212-222: Tests verify local logic, not actual handler behavior.These tests verify that
[i.strip().lower() for i in "...".split(",")]works correctly, but this pattern is implemented inline in the test—not exercised against the actual handler.Looking at the handler code, it does
include_related = query_params.get("include", "").split(",")and then checksif "managers" in include_related, but there's no.strip().lower()normalization visible in the provided context.Consider either:
- Converting these to integration tests that call
handle_organizationswithquery_params={"include": "Managers, Tags"}and verify the handler correctly normalizes and processes them, or- Documenting that these tests verify expected normalization logic that should be implemented in the handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_organizations.py` around lines 212 - 222, The tests only exercise local list comprehension instead of the handler behavior; update either the tests or the handler so normalization is actually covered: either change the tests to call handle_organizations with query_params={"include":"Managers, Tags"} and assert the handler inspects include_related (the value produced from query_params.get("include","").split(",")) after normalizing, or implement normalization inside the handler by replacing include_related = query_params.get("include","").split(",") with a normalized version (e.g., [i.strip().lower() for i in include_related_raw.split(",")]) so checks like if "managers" in include_related work as intended; reference include_related, handle_organizations, and query_params when applying the fix.
10-10: Unused import.
jsonis imported but never used in this test file.🧹 Proposed fix
import pytest import sys -import json from pathlib import Path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_organizations.py` at line 10, The import "json" is unused in tests/test_organizations.py; remove the unused import statement (the line containing "import json") so the file no longer imports json unnecessarily and ensure no other references to the symbol remain (check functions in this module like any test_* functions) before committing.
81-85: Unused helper function.
make_response_jsonis defined but never used in any test. Consider removing it to avoid confusion, or add tests that utilize it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_organizations.py` around lines 81 - 85, The helper function make_response_json is defined but unused; either remove make_response_json from tests/test_organizations.py to avoid dead code, or update it to correctly simulate a Response by implementing a .json() method (e.g., return a MagicMock whose .json() returns the provided data) and replace existing manual mocks in tests with calls to make_response_json so tests use the helper; reference the function name make_response_json when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_organizations.py`:
- Around line 120-146: The tests expect 400 responses for invalid query params
but the handler handle_organizations currently silently ignores invalid "type"
and coerces any non-matching "is_active" to false; update handle_organizations
in handlers.organizations to explicitly validate both params and return a 400
response when invalid: for "type" check that org_type is one of
["company","nonprofit","education"] and if provided but not in the set return
400, and for "is_active" only accept known truthy/falsey strings (e.g.,
"true","1","yes","false","0","no") and return 400 for anything else instead of
coercing to 0; make sure to modify the code paths around the existing org_type
conditional and the is_active conversion logic so tests pass.
- Around line 194-206: The handler leaks raw exception text into 503 responses;
update the error handling in handle_organizations (the block that calls
get_db_safe) so that when an Exception occurs you do not pass str(e) into
error_response/json_response; instead return a sanitized message like "Service
unavailable" (e.g., call error_response("Service unavailable", status=503) or
otherwise map the exception to a generic message), ensuring
error_response/json_response are invoked with the generic string and not the raw
exception from get_db_safe.
- Around line 228-238: The test expects an exported async function
_get_organization_stats(db, org_id) but the stats logic is currently inline in
handle_organizations; extract the block that computes domain_count, bug_count,
verified_bug_count, manager_count into a new async function named
_get_organization_stats(db, org_id) in handlers.organizations, have it accept
the DB and org id and return the stats dict, and then replace the inline stats
logic in handle_organizations (the /stats endpoint) with a call to await
_get_organization_stats(db, org_id).
---
Nitpick comments:
In `@tests/test_organizations.py`:
- Around line 212-222: The tests only exercise local list comprehension instead
of the handler behavior; update either the tests or the handler so normalization
is actually covered: either change the tests to call handle_organizations with
query_params={"include":"Managers, Tags"} and assert the handler inspects
include_related (the value produced from
query_params.get("include","").split(",")) after normalizing, or implement
normalization inside the handler by replacing include_related =
query_params.get("include","").split(",") with a normalized version (e.g.,
[i.strip().lower() for i in include_related_raw.split(",")]) so checks like if
"managers" in include_related work as intended; reference include_related,
handle_organizations, and query_params when applying the fix.
- Line 10: The import "json" is unused in tests/test_organizations.py; remove
the unused import statement (the line containing "import json") so the file no
longer imports json unnecessarily and ensure no other references to the symbol
remain (check functions in this module like any test_* functions) before
committing.
- Around line 81-85: The helper function make_response_json is defined but
unused; either remove make_response_json from tests/test_organizations.py to
avoid dead code, or update it to correctly simulate a Response by implementing a
.json() method (e.g., return a MagicMock whose .json() returns the provided
data) and replace existing manual mocks in tests with calls to
make_response_json so tests use the helper; reference the function name
make_response_json when making the change.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 650e4fbd-ad4a-438d-b16e-391c78278dec
📒 Files selected for processing (1)
tests/test_organizations.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/organizations.py`:
- Around line 121-123: The query payloads are leaking user account emails via
the .values(...) projection (specifically the 'users.email' field used in the
organization managers and organization details queries); remove 'users.email'
from those .values() calls (the call listing 'users.id', 'users.username',
'users.user_avatar', 'users.total_score', 'organization_managers.created AS
joined_as_manager' and the separate projection at lines ~188-192) so email is
not returned, and update any downstream code that expects email in the response
(e.g., response building logic for managers/org details) to stop referencing
users.email or to pull public contact fields from an allowed metadata source
instead.
- Around line 159-163: The integrations query in organizations.py is returning
raw webhook_url (via OrganizationIntegration.objects(...).values(...)) which
should not be exposed; update the code that builds the integrations response
(the integrations variable) to omit webhook_url from the .values() list or, if
the UI needs an indicator, post-process the returned records to replace
webhook_url with a masked value (e.g., null or a fixed masked string or a
boolean like has_webhook) before returning the response so raw secrets are never
sent.
- Around line 68-69: After converting org_id to org_id_int, validate that the
organization actually exists before dispatching any nested resource handlers:
query the data layer (e.g., call get_organization_by_id(org_id_int) or use your
Organization model/ORM to fetch by id) and if the lookup returns None,
raise/return a 404 (e.g., HTTPException(status_code=404, detail="Organization
not found")); if found, continue and pass the retrieved org object to subsequent
domain/bugs/managers/tags/integrations/stats handlers instead of letting them
return empty results for a non-existent org.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fa480932-2276-4d07-a5ad-228ccaa81229
📒 Files selected for processing (1)
src/handlers/organizations.py
Contributor
Author
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13224
Merge Readiness Assessment — PR
|
| # | Location | Issue |
|---|---|---|
| 1 | ~Lines 121–123 | Security – email data leak: .values() projections include users.email, which means user account emails are returned in API responses for managers/org details endpoints. |
| 2 | ~Lines 159–163 | Security – secret exposure: The integrations query returns raw webhook_url, which could be a sensitive credential. Should be omitted or masked (e.g., replaced with a boolean has_webhook). |
| 3 | ~Lines 68–69 | Logic gap – missing 404: After parsing org_id_int, the handler never verifies the organization exists before dispatching to nested resource handlers (/domains, /bugs, /managers, etc.). A non-existent org silently returns empty results instead of a proper 404. |
In tests/test_organizations.py
| # | Location | Issue |
|---|---|---|
| 4 | ~Lines 120–146 | Test/handler mismatch: Tests assert 400 responses for invalid is_active and type query params, but the handler currently silently ignores invalid type values and coerces unrecognized is_active to false. The handler validation logic needs to be added to make these tests pass. |
| 5 | ~Lines 194–206 | Exception message leakage: The 503 error path returns the raw exception string (str(e)) in the response body. Tests validate that internal errors are not leaked, but the handler currently leaks them. A generic sanitized message (e.g., "Service unavailable") should be returned instead. |
| 6 | ~Lines 228–238 | Missing extraction of _get_organization_stats: Tests import and call _get_organization_stats as a standalone exported function, but this logic is currently inlined inside handle_organizations. It needs to be extracted into its own async def _get_organization_stats(db, org_id_int) function. |
🟡 Nitpick Issues (minor, non-blocking)
| # | Location | Issue |
|---|---|---|
| N1 | Line 10 | Unused import json in tests/test_organizations.py |
| N2 | Lines 81–85 | make_response_json helper defined but never used in any test |
| N3 | Lines 212–222 | TestIncludeTokenNormalization tests only verify a local list comprehension — not the actual handler behavior. These should either call handle_organizations with mixed-case/padded include params or the handler should implement the normalization so the tests are meaningful. |
✅ What's in good shape
- The overall test structure (MockDB pattern,
patch-based injection) mirrors the existingtests/test_orm.pypattern — consistent approach. ALLOWED_ORG_TYPESconstant is well-tested.- DB failure → 503 path is a good addition; just needs the exception message sanitization fix.
- The ORM refactor in
src/handlers/organizations.pyis a significant improvement over raw SQL.
Summary
The PR cannot be merged as-is. The two security issues (email leak and webhook URL exposure) are the most critical and must be addressed. The handler also needs input validation for is_active/type (issue #4), exception sanitization for 503 (issue #5), and extraction of _get_organization_stats (issue #6) to align with what the tests expect.
Would you like me to help plan or implement any of these fixes?
Contributor
Author
|
@coderabbitai resume |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_organizations.py (1)
206-215: Consider testing normalization through the handler or documenting scope.These tests verify the string normalization pattern (
strip().lower()) in isolation rather than testing actual handler behavior with theincludequery parameter. This is acceptable as a unit test, but consider adding an integration-style test that callshandle_organizationswith mixed-caseincludetokens to verify end-to-end normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_organizations.py` around lines 206 - 215, Current tests only assert the local normalization logic; add an integration-style test that calls the actual handler to confirm end-to-end behavior: create a new test that invokes handle_organizations (or the HTTP handler wrapper used in tests) with an include query containing mixed-case and spaced tokens like "Managers, Tags, STATS" and assert the response/behavior uses the normalized tokens (e.g., includes "managers", "tags", "stats" in whatever field or code path handle_organizations exposes). Place this alongside TestIncludeTokenNormalization and reuse existing test fixtures/mocks so the test exercises the handler code path rather than only the list comprehension.src/handlers/organizations.py (1)
71-76: Consider distinguishing DB errors from missing organizations.If the count query fails (e.g., network timeout), the exception is silently caught and
org_existsis set to 0, resulting in a 404. This could mask real database issues and confuse debugging during outages.♻️ Optional: Let DB errors propagate to return 500
# Verify org exists before dispatching to sub-resource routes - try: - org_exists = await Organization.objects(db).filter(id=org_id_int).count() - except Exception: - org_exists = 0 + org_exists = await Organization.objects(db).filter(id=org_id_int).count() if not org_exists: return error_response("Organization not found", status=404)This lets the outer exception handlers (lines 91, 113, etc.) handle DB failures consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/organizations.py` around lines 71 - 76, The current try/except around Organization.objects(db).filter(id=org_id_int).count() swallows all DB errors and treats them as "not found"; remove the broad exception handling or change it to catch only expected non-DB errors, so real DB exceptions propagate (or log and re-raise) instead of setting org_exists = 0; keep the subsequent check that if not org_exists then return error_response("Organization not found", status=404). This ensures failures in Organization.objects(...).filter(...).count() surface as 500s to the outer handlers rather than false 404s.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/organizations.py`:
- Around line 268-286: The code calls a non-existent QuerySet.filter_or in
src/handlers/organizations.py; add a reusable filter_or method to the QuerySet
class in src/libs/orm.py that accepts kwargs or a dict, stores OR clauses (e.g.
in a new attribute like _or_conditions), and ensures when building the SQL/WHERE
clause the stored OR conditions are combined with OR (not AND) and correctly
parameterized alongside existing filter/exclude logic; update QuerySet's SQL
generation/compose method to include _or_conditions so calls such as
Organization.objects(...).filter_or(...) used in both the main query and
count_qs work without runtime errors.
---
Nitpick comments:
In `@src/handlers/organizations.py`:
- Around line 71-76: The current try/except around
Organization.objects(db).filter(id=org_id_int).count() swallows all DB errors
and treats them as "not found"; remove the broad exception handling or change it
to catch only expected non-DB errors, so real DB exceptions propagate (or log
and re-raise) instead of setting org_exists = 0; keep the subsequent check that
if not org_exists then return error_response("Organization not found",
status=404). This ensures failures in
Organization.objects(...).filter(...).count() surface as 500s to the outer
handlers rather than false 404s.
In `@tests/test_organizations.py`:
- Around line 206-215: Current tests only assert the local normalization logic;
add an integration-style test that calls the actual handler to confirm
end-to-end behavior: create a new test that invokes handle_organizations (or the
HTTP handler wrapper used in tests) with an include query containing mixed-case
and spaced tokens like "Managers, Tags, STATS" and assert the response/behavior
uses the normalized tokens (e.g., includes "managers", "tags", "stats" in
whatever field or code path handle_organizations exposes). Place this alongside
TestIncludeTokenNormalization and reuse existing test fixtures/mocks so the test
exercises the handler code path rather than only the list comprehension.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bcdbf5b-29f4-4ad7-b96f-5d7115bd69b6
📒 Files selected for processing (2)
src/handlers/organizations.pytests/test_organizations.py
owasp-blt
bot
added
unresolved-conversations: 2
and removed
unresolved-conversations: 0
labels
✅ Actions performedReviews resumed. |
1 similar comment
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/conftest.py (1)
14-14: Redundant import ofsys.
sysis already imported at line 5. Consider reusing it instead of re-importing as_sys.♻️ Suggested fix
-import sys as _sys import types as _types -if "workers" not in _sys.modules: +if "workers" not in sys.modules: _workers_mod = _types.ModuleType("workers") ... - _sys.modules["workers"] = _workers_mod + sys.modules["workers"] = _workers_mod🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` at line 14, Remove the redundant import "import sys as _sys" and reuse the existing "sys" import already present earlier in the file; update any occurrences of "_sys" in this module to "sys" so there are no duplicate imports and all references use the single "sys" symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conftest.py`:
- Around line 26-30: The test shim Response.json method in tests/conftest.py
currently ignores the headers kwarg; update the staticmethod json in that file
to accept and forward headers (and any other kwargs) into the _Response
constructor so headers passed by production handlers (e.g., cors_headers()) are
preserved; locate the json method and change the construction of
_Response(_json.dumps(data), status) to include the headers from kwargs when
calling _Response.
---
Nitpick comments:
In `@tests/conftest.py`:
- Line 14: Remove the redundant import "import sys as _sys" and reuse the
existing "sys" import already present earlier in the file; update any
occurrences of "_sys" in this module to "sys" so there are no duplicate imports
and all references use the single "sys" symbol.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6009ee4f-5545-4716-af3e-a50121510836
📒 Files selected for processing (3)
src/libs/orm.pysrc/models.pytests/conftest.py
owasp-blt
bot
added
unresolved-conversations: 2
and removed
unresolved-conversations: 2
labels
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/organizations.py`:
- Line 57: Replace all plain print(...) statements that interpolate exception
objects (e.g., print(f"Database connection error: {e}")) with a safe logging
call that does not emit raw exception contents; for example, use the module's
logger (or processLogger) to log a sanitized message like "Database connection
error while handling [operation]" and avoid including the exception string or
any sensitive fields, and if you need diagnostics keep exc_info=False (or log
only exception.__class__.__name__) so the secret connection string is not
written to logs; update every occurrence (the prints reported and similar prints
at other locations) to follow this pattern.
- Around line 70-73: The Organization existence probe using
Organization.objects(db).filter(id=org_id_int).count() can raise DB errors
outside the handler’s existing id-path error handling; wrap that call in the
same try/except/error-mapping used by the rest of the id branch (e.g., mirror
the handler’s existing error handling pattern) so any exception is caught and a
sanitized JSON error is returned (use the same error_response pathway), ensuring
you still return error_response("Organization not found", status=404) when count
== 0 but convert DB exceptions into the handler’s standard JSON error response
instead of letting them propagate.
In `@tests/test_organizations.py`:
- Around line 206-215: The tests in TestIncludeTokenNormalization only recompute
the normalization locally and never hit the handler branch in
handle_organizations that builds include_related; update the tests (e.g.,
test_include_tokens_lowercase and test_include_tokens_strip_whitespace) to
invoke the async handler path instead of duplicating the list comprehension:
call the organizations handler (handle_organizations) via the test client or
directly await the async handler with a request that includes the include query
(e.g., ?include=Managers,Tags,STATS) while ensuring the org existence check
passes (mock the org lookup to avoid the 404) so the include_related
construction runs, then assert the handler used normalized tokens (e.g.,
response or internal call saw "managers","tags","stats" and no " tags"). Ensure
the tests reference the handler name handle_organizations and the test
class/method names to locate the code.
- Around line 143-154: The test test_valid_is_active_true_accepted currently
asserts only that response.status_code != 400 which still passes on 500; change
the assertion to verify a successful status (e.g., assert response.status_code
== 200 or assert 200 <= response.status_code < 300) after calling
handle_organizations so the happy path is enforced; apply the same tightening to
the other similar test (the one around lines 157-168) that uses
handle_organizations and response.status_code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8b316a0-00a6-4024-a655-8813ea9392c7
📒 Files selected for processing (3)
src/handlers/organizations.pysrc/utils.pytests/test_organizations.py
owasp-blt
bot
added
unresolved-conversations: 6
and removed
unresolved-conversations: 2
labels
Contributor
Author
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
🧩 Analysis chain🏁 Script executed: Length of output: 59614
Updated Merge Readiness Assessment — PR
|
| # | Issue | Addressed In |
|---|---|---|
| 1 | Email PII leak (users.email) |
Commit 0dc8742 |
| 2 | Webhook URL exposure in /integrations |
Commit 0dc8742 |
| 3 | Missing org existence check (404 for non-existent orgs) | Commit 0dc8742 |
| 4 | _get_organization_stats extracted as standalone function |
Commit 0d960fd |
| 5 | Input validation returning 400 for invalid is_active/type |
Commit 0d960fd |
| 6 | filter_or() method added to ORM QuerySet |
Commit 38edd69 |
| 7 | Raw exceptions printed to logs (sanitized) | Commits bd65f89→8b40cbd |
| 8 | Org existence probe wrapped in try/except |
Commits bd65f89→8b40cbd |
| 9 | Response.json() discarding headers kwarg |
Commits bd65f89→ac13628 |
| 10 | Happy-path assertions (!= 400 → == 200) |
Commits bd65f89→8b40cbd |
| 11 | Include-normalization tests exercising handler | Commits bd65f89→ac13628 |
🔴 Still Unresolved (1 item)
| # | Location | Issue |
|---|---|---|
| 1 | tests/test_organizations.py Line 200 |
503 response leaks raw exception message — the review comment still has no resolution marker and resolved: null. The test asserts the error body does not contain the raw exception string, but this fix may have landed in commit 0dc8742 ("sanitiz…") without being detected. Needs confirmation. |
🟡 Nitpicks (non-blocking, still open)
| # | Location | Issue |
|---|---|---|
| N1 | tests/conftest.py Line 14 |
Redundant import sys as _sys — sys is already imported earlier |
⚙️ Process Note
The two most recent commits (ac13628 at 20:17Z and 8b40cbd at 20:44Z) were pushed after the last review round (submitted at 20:28Z covering up to bd65f898). The review decision remains CHANGES_REQUESTED because those commits have not yet been reviewed. I'll trigger a fresh review now to assess the final state and confirm whether the 503 comment is fully resolved.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 42179
Merge Readiness Assessment — PR
|
| # | File | Line | Severity | Summary |
|---|---|---|---|---|
| 1 | tests/test_organizations.py |
140 | 🟠 Major | Happy-path assertions still use != 400 instead of == 200 |
| 2 | tests/test_organizations.py |
200 | 🔴 Critical | 503 test may still pass even if exception is leaked (related to #1 above) |
| 3 | tests/test_organizations.py |
247 | 🔴 Critical | _get_organization_stats extraction — thread not resolved |
| 4 | src/handlers/organizations.py |
69 | 🟠 Major | Org existence check thread not resolved |
| 5 | src/handlers/organizations.py |
— | 🟠 Major | Email PII leak thread not resolved |
| 6 | src/handlers/organizations.py |
— | 🟠 Major | webhook_url exposure thread not resolved |
| 7 | src/handlers/organizations.py |
287 | 🔴 Critical | filter_or in ORM thread not resolved |
| 8 | tests/conftest.py |
29 | 🟡 Minor | Response.json() headers kwarg thread not resolved |
| 9 | src/handlers/organizations.py |
— | 🟠 Major | print(f"... {e}") raw exception logging thread not resolved |
| 10 | src/handlers/organizations.py |
77 | 🟠 Major | Existence probe error-handling thread not resolved |
| 11 | tests/test_organizations.py |
— | 🟠 Major | Happy-path != 400 assertion thread not resolved |
| 12 | tests/test_organizations.py |
— | 🟠 Major | Include-normalization tests not exercising handler thread not resolved |
Note: Some of these threads may correspond to code that was already fixed in commits — but GitHub does not auto-resolve threads. They need to be manually resolved once the fix is confirmed, or a new review pass needs to mark them addressed.
✅ What Looks Good Since Last Assessment
| Item | Status |
|---|---|
import sys as _sys redundancy in conftest.py |
✅ Removed |
workers.Response.json now forwards headers kwarg |
✅ Fixed |
_get_organization_stats extracted as standalone function |
✅ Done (commit 0d960fd) |
filter_or() added to ORM QuerySet |
✅ Done (commit 38edd69) |
| Trivial normalization unit tests replaced with integration test | ✅ Done (commit 95ee8e5d, unreviewed) |
⚙️ Next Steps to Reach Merge Readiness
- Fix the 503 leak — replace
str(e)with"Service unavailable"in theexceptblock (confirmed still present). - Resolve GitHub review threads — go through the 12 open threads and close each one that has been addressed by a commit.
- Trigger a fresh review — the latest commit (
95ee8e5d) has not been reviewed yet.
Would you like me to trigger a review of the latest commit now?
Contributor
Author
|
@coderabbitai trigger a review of the latest commit now |
|
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 43786
Merge Readiness Assessment — PR
|
| # | File | Topic | Status |
|---|---|---|---|
| 1 | organizations.py:34 |
503 raw exception leak | 🔴 Confirmed still in code |
| 2 | test_organizations.py:154 |
Happy-path != 400 → should be == 200 |
Likely fixed in commit 8b40cbd |
| 3 | test_organizations.py:215 |
Include-normalization tests not hitting handler | Addressed in commit 95ee8e5d (unreviewed) |
| 4 | organizations.py:69 |
Org existence check (404) | Fixed in commit 0dc8742 |
| 5 | organizations.py:123 |
Email PII leak | Fixed in commit 0dc8742 |
| 6 | organizations.py:163 |
Webhook URL exposure | Fixed in commit 0dc8742 |
| 7 | organizations.py:287 |
filter_or ORM method |
Fixed in commit 38edd69 |
| 8 | conftest.py:30 |
Response.json() drops headers |
Fixed in commit ac13628 |
| 9 | organizations.py:57 |
Raw exception in print() |
Fixed in commit 8b40cbd |
| 10 | organizations.py:73 |
Existence probe error handling | Fixed in commit 8b40cbd |
| 11 | test_organizations.py:154 |
Assert == 200 not != 400 |
Likely fixed in 8b40cbd |
| 12 | test_organizations.py:215 |
Normalization not exercising handler | Fixed in commit 95ee8e5d |
✅ What's Confirmed Good
| Item | Commit |
|---|---|
| Email PII removed | 0dc8742 |
| Webhook URL removed | 0dc8742 |
| Org existence check (404) | 0dc8742 |
_get_organization_stats extracted |
0d960fd |
Input validation for is_active/type |
0d960fd |
filter_or() added to ORM |
38edd69 |
Response.json() forwards headers |
ac13628 |
Redundant import sys as _sys removed |
ac13628 |
| Exception logging sanitized | 8b40cbd |
status_code attribute on MockResponse |
8b40cbd |
⚙️ Required Next Steps
- Fix the 503 leak — change
error_response(str(e), status=503)→error_response("Service unavailable", status=503)atorganizations.pyline 34. - Resolve GitHub threads — manually close the 11 comment threads that have already been addressed by commits.
- Trigger a new review — after fixing
#1, trigger a review of the new commit so theCHANGES_REQUESTEDdecision can be updated.
Would you like me to trigger a review of commit 95ee8e5d now?
…e 503 message, remove unused test helpers
… add missing models, fix conftest workers shim
…st for include token normalization, add status_code to MockResponse
…esponse.json shim
…pt, tighten test assertions to 200
…ches include normalization code path
SatishKumar620
force-pushed
the
test/add-organizations-handler-tests
branch
from
95ee8e5 to
b713a07
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/libs/orm.py (1)
138-139: Keepfilter_or()groups composable.A second
filter_or()call currently widens the same OR bucket, soqs.filter_or(a=1, b=2).filter_or(c=3, d=4)becomes(a OR b OR c OR d)instead of(a OR b) AND (c OR d). Consider storing OR clauses as per-call groups and emitting one parenthesized group per call.Also applies to: 154-154, 183-194, 351-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/orm.py` around lines 138 - 139, The current implementation appends all OR conditions into a single list self._or_filters which causes successive calls to filter_or(...) to merge into one big OR; change it so each filter_or(...) call creates and stores a separate group (e.g., append a list/tuple representing that call) instead of flattening into self._or_filters, then have the query builder (where ORs are emitted — look for code that reads _or_filters and the methods filter_or and build/query construction) render each stored group as a parenthesized OR clause and join those groups with AND; update any related handling in methods referencing _or_filters (including the logic around lines indicated) to treat it as a list of groups rather than a flat list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/organizations.py`:
- Around line 105-107: The .values() call in organizations.py is using SQL-style
aliases like "domains.name AS domain_name" and "organization_managers.created AS
joined_as_manager" which ValueError because QuerySet.values() only accepts
identifier-shaped names; replace those aliased entries with ORM lookups (e.g.
use "domains__name" and "organization_managers__created" inside the .values(...)
call where 'bugs.id', 'bugs.url', etc. are listed) and then, after calling
.all() (or evaluating the queryset), rename those keys in the resulting dicts to
"domain_name" and "joined_as_manager" respectively; apply the same change for
the other occurrence around lines 130-132 so the query no longer uses "AS"
aliases and the final payload uses the intended key names.
---
Nitpick comments:
In `@src/libs/orm.py`:
- Around line 138-139: The current implementation appends all OR conditions into
a single list self._or_filters which causes successive calls to filter_or(...)
to merge into one big OR; change it so each filter_or(...) call creates and
stores a separate group (e.g., append a list/tuple representing that call)
instead of flattening into self._or_filters, then have the query builder (where
ORs are emitted — look for code that reads _or_filters and the methods filter_or
and build/query construction) render each stored group as a parenthesized OR
clause and join those groups with AND; update any related handling in methods
referencing _or_filters (including the logic around lines indicated) to treat it
as a list of groups rather than a flat list.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c56d3a33-0a66-4629-a486-59a341ba2fef
📒 Files selected for processing (6)
src/handlers/organizations.pysrc/libs/orm.pysrc/models.pysrc/utils.pytests/conftest.pytests/test_organizations.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/utils.py
- src/models.py
- tests/test_organizations.py
- tests/conftest.py
owasp-blt
bot
added
unresolved-conversations: 2
and removed
unresolved-conversations: 1
labels
📊 Monthly LeaderboardHi @SatishKumar620! Here's how you rank for April 2026:
Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run |
Contributor
Author
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 47295
Merge Readiness Assessment — PR
|
| Item | Status |
|---|---|
pytest CI |
✅ Passing (13s) |
| CodeRabbit check | ✅ Pass |
Email PII (users.email) removed |
✅ Fixed |
| Webhook URL exposure removed | ✅ Fixed |
| Org existence check (404) added | ✅ Fixed |
_get_organization_stats extracted |
✅ Fixed |
Input validation for is_active/type (→ 400) |
✅ Fixed |
filter_or() added to ORM |
✅ Fixed |
Response.json() forwards headers |
✅ Fixed |
Exception logging sanitized (type(e).__name__) |
✅ Fixed |
🔴 Confirmed Blocking Bugs (in current source)
Bug 1 — Raw exception leak in 503 response (line 34 of organizations.py)
The source code still contains this exact line:
except Exception as e:
return error_response(str(e), status=503) # ← raw exception string still leakedFix required:
except Exception as e:
print(f"DB connection error: {type(e).__name__}")
return error_response("Service unavailable", status=503)Bug 2 — SQL-style aliases in .values() cause runtime ValueError (lines ~105–132 of organizations.py) ⚠️ Newly confirmed in latest review
The handler uses SQL-style AS aliases in .values() calls, e.g.:
.values("domains.name AS domain_name", "organization_managers.created AS joined_as_manager", ...)QuerySet.values() only accepts plain identifier-shaped names. These calls will raise a ValueError at runtime. They need to be replaced with ORM-style lookups (e.g., "domains__name") and the aliases applied in post-processing:
# Instead of:
.values("domains.name AS domain_name")
# Use:
.values("domains__name")
# Then rename the key in the result dict: result["domain_name"] = result.pop("domains__name")🟡 Nitpick (Non-Blocking)
| # | Location | Issue |
|---|---|---|
| N1 | src/libs/orm.py ~lines 138–139 |
filter_or() composability: successive filter_or() calls merge all OR conditions into one flat group instead of emitting one parenthesized group per call. qs.filter_or(a=1).filter_or(b=2) becomes (a OR b) instead of (a) AND (b). |
📋 Open GitHub Comment Threads (13 total)
Most of these correspond to already-fixed issues but were never manually resolved on GitHub. You should resolve the ones listed below as fixed, and address the two that remain open code bugs:
| Thread | File | Status |
|---|---|---|
| 503 exception leak | organizations.py:34 |
🔴 Still in code |
SQL aliases in .values() |
organizations.py:107 |
🔴 Still in code |
| Org existence check | organizations.py:69 |
✅ Fixed — resolve thread |
| Email PII leak | organizations.py |
✅ Fixed — resolve thread |
| Webhook URL exposure | organizations.py |
✅ Fixed — resolve thread |
filter_or() ORM |
organizations.py:287 |
✅ Fixed — resolve thread |
Response.json() headers |
conftest.py:30 |
✅ Fixed — resolve thread |
Raw print(f"...{e}") |
organizations.py |
✅ Fixed — resolve thread |
| Existence probe error handling | organizations.py:77 |
✅ Fixed — resolve thread |
Happy-path != 400 assertions |
test_organizations.py |
✅ Fixed — resolve thread |
| Include-normalization tests | test_organizations.py |
✅ Fixed — resolve thread |
| Input validation 400 | test_organizations.py:140 |
✅ Fixed — resolve thread |
_get_organization_stats extraction |
test_organizations.py:247 |
✅ Fixed — resolve thread |
⚙️ Required Next Steps to Reach Merge Readiness
- Fix Bug 1 — replace
error_response(str(e), status=503)witherror_response("Service unavailable", status=503)atorganizations.pyline 34. - Fix Bug 2 — replace SQL-style
ASaliases in.values()calls with ORM-style field lookups ("domains__name","organization_managers__created") and rename keys post-query. - Resolve GitHub threads — manually mark the 11 threads that have been addressed by commits as resolved on GitHub to clear the
CHANGES_REQUESTEDstatus backlog. - Trigger a new review — after fixing items 1 and 2, request a fresh review to confirm no remaining issues.
owasp-blt
bot
added
unresolved-conversations: 0
and removed
unresolved-conversations: 2
labels
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
1 participant
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.
Summary
Follow-up to PR #64 — adds smoke tests for the
organizations handler.
Tests Added (
tests/test_organizations.py)TestAllowedOrgTypes— validates ALLOWED_ORG_TYPES constantTestOrganizationInputValidation:TestDBConnectionFailure:TestIncludeTokenNormalization— lowercase + stripTestGetOrganizationStats— all 4 keys presentApproach
Uses same MockDB pattern as
tests/test_orm.py—no real database or HTTP server required.
Summary by CodeRabbit
New Features
Bug Fixes
Tests