fix(bugs): prevent unauthenticated mass-assignment in POST /bugs#42
fix(bugs): prevent unauthenticated mass-assignment in POST /bugs#42Kunal241207 wants to merge 4 commits intoOWASP-BLT:mainfrom
Conversation
|
👋 Thanks for opening this pull request, @Kunal241207! Before your PR is reviewed, please ensure:
🔍 Our team will review your PR shortly. If you have questions, feel free to ask in the comments. 🚀 Keep up the great work! — OWASP BLT |
📊 Monthly LeaderboardHi @Kunal241207! Here's how you rank for March 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 |
owasp-blt
bot
added
unresolved-conversations: 0
needs-peer-review
|
👋 Hi @Kunal241207! 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!
|
WalkthroughSanitized POST body using a positive allowlist; validated and normalized Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Bug Handler
participant Sanitizer as Sanitizer/Validator
participant DB as Database
Client->>Handler: POST /bugs (body, headers)
Handler->>Sanitizer: Apply allowlist, validate/trim `url` and `description`
Sanitizer-->>Handler: sanitized_body or validation error
Handler->>Handler: Extract reporter_ip from CF-Connecting-IP
Handler->>Handler: Derive user_id from request context
Handler->>DB: INSERT using sanitized_body + reporter_ip + user_id + server-managed defaults
DB-->>Handler: insert id / result
Handler-->>Client: 201 Created with created bug or 400 Validation 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: 1
🧹 Nitpick comments (1)
tests/test_bugs_security.py (1)
23-52: Parametrize this so each protected field is asserted independently.Right now the test only proves that a payload containing all privileged fields is rejected. It would still pass if three of those fields became writable and only one remained blocked. A per-field case will make the regression guard precise.
Example refactor
`@pytest.mark.asyncio` -async def test_create_bug_rejects_privileged_fields(monkeypatch): +@pytest.mark.parametrize( + ("privileged_field", "value"), + [ + ("verified", True), + ("user", 10), + ("closed_by", 20), + ("reporter_ip_address", "1.2.3.4"), + ], +) +async def test_create_bug_rejects_privileged_fields(monkeypatch, privileged_field, value): async def _fake_get_db_safe(_env): return object() monkeypatch.setattr(bugs_handler, "get_db_safe", _fake_get_db_safe) request_body = json.dumps( { "url": "https://example.com/vuln", "description": "test", - "verified": True, - "user": 10, - "closed_by": 20, - "reporter_ip_address": "1.2.3.4", + privileged_field: value, } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bugs_security.py` around lines 23 - 52, Replace the single-case test_create_bug_rejects_privileged_fields with a parameterized pytest (use `@pytest.mark.parametrize`) that iterates over each protected field name (e.g., "verified", "user", "closed_by", "reporter_ip_address") and for each iteration builds a request_body containing only that one protected field plus the required normal fields, then calls bugs_handler.handle_bugs (same monkeypatch of get_db_safe) and asserts response.status == 400 and that payload["message"] contains "server-managed"; keep the same MockRequest/MockEnv usage and error expectations but ensure each protected field is asserted independently.
🤖 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/bugs.py`:
- Around line 196-204: The create handler currently uses a blacklist
(privileged_fields/provided_privileged_fields) but still copies fields like
views, status, is_hidden, rewarded from body; replace this with a positive
allowlist: define client_writable_fields (e.g. title, description, steps, etc.),
build a sanitized_body by copying only those keys from body, and explicitly set
server-managed fields (views=0, is_hidden=False, rewarded=False, status="open",
verified=False, user=server-side-id, closed_by=None, reporter_ip_address=request
IP) before persisting; remove any code that reads
views/status/is_hidden/rewarded from body and ensure the same allowlist approach
is used in the create path and the referenced block around
privileged_fields/provided_privileged_fields.
---
Nitpick comments:
In `@tests/test_bugs_security.py`:
- Around line 23-52: Replace the single-case
test_create_bug_rejects_privileged_fields with a parameterized pytest (use
`@pytest.mark.parametrize`) that iterates over each protected field name (e.g.,
"verified", "user", "closed_by", "reporter_ip_address") and for each iteration
builds a request_body containing only that one protected field plus the required
normal fields, then calls bugs_handler.handle_bugs (same monkeypatch of
get_db_safe) and asserts response.status == 400 and that payload["message"]
contains "server-managed"; keep the same MockRequest/MockEnv usage and error
expectations but ensure each protected field is asserted independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e9cf8556-51cd-4dee-ba25-f42133364f63
📒 Files selected for processing (2)
src/handlers/bugs.pytests/test_bugs_security.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bugs.py`:
- Around line 214-225: The code reads body["url"] into url and calls len(url)
before ensuring it's a string, which will raise on non-string payloads (e.g.,
url: 123); update the handler to first check that url is an instance of str
(e.g., using isinstance(url, str)) and return error_response("URL must be a
string", status=400) if not, then perform the existing length check and
urlparse/scheme validation (references: variable url, error_response, and the
urlparse block).
- Around line 196-213: The handler currently uses client_writable_fields and
builds sanitized_body to silently drop server-managed fields; instead detect if
any disallowed/server-managed fields (e.g., "verified", "user", "closed_by",
"reporter_ip_address" and any other fields the server owns) are present in the
incoming body and immediately return an HTTP 400 error; update the logic around
client_writable_fields/sanitized_body in the request handling function in
src/handlers/bugs.py to compute forbidden = set(body.keys()) -
client_writable_fields, and if forbidden is non-empty respond with a 400 and a
message listing those forbidden fields rather than proceeding to create the
resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 05e06a76-947a-4223-bed5-eedbe7381ef0
📒 Files selected for processing (2)
src/handlers/bugs.pytests/test_bugs_security.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_bugs_security.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bugs.py`:
- Around line 222-229: The code strips and validates url into the local variable
url but later still binds sanitized_body.get("url"), so persist the normalized
value by using the local url variable in the INSERT/bind instead of
sanitized_body.get("url"); also validate description before the INSERT by
fetching description = sanitized_body.get("description"), confirming it's a
string and that description.strip() is non-empty, returning
error_response("Description is required", status=400) for blank values, and then
use the validated description variable when binding to the DB.
- Around line 284-300: The binds are converting valid numeric zeroes to None via
the "... or None" pattern; update the numeric fields (e.g., the uses of
sanitized_body.get("score") and sanitized_body.get("cve_score") in this
parameter list, and any other numeric binds using "or None") to not use "or
None" — instead pass the raw .get(...) result or use a presence check like
sanitized_body.get("score") if "score" in sanitized_body else None so that
explicit 0 is preserved while still returning None when the key is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 17172594-f6ad-4cb5-bb98-f2d56c6b45da
📒 Files selected for processing (2)
src/handlers/bugs.pytests/test_bugs_security.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_bugs_security.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/handlers/bugs.py (1)
273-281: Consider simplifying attribute access pattern.The
hasattr/getattrpattern can be simplified usinggetattrwith a default value. This also addresses the static analysis hint about usinggetattrwith constant attributes.🛠️ Suggested simplification
- # Server-side identity (if an auth layer attaches one to request). - user_id = None - if hasattr(request, "user_id"): - user_id = getattr(request, "user_id") - elif hasattr(request, "user"): - request_user = getattr(request, "user") - if isinstance(request_user, dict): - user_id = request_user.get("id") - else: - user_id = getattr(request_user, "id", None) + # Server-side identity (if an auth layer attaches one to request). + user_id = getattr(request, "user_id", None) + if user_id is None: + request_user = getattr(request, "user", None) + if request_user is not None: + if isinstance(request_user, dict): + user_id = request_user.get("id") + else: + user_id = getattr(request_user, "id", None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/bugs.py` around lines 273 - 281, The code repeatedly uses hasattr/getattr to extract a user id from request; simplify by using getattr with defaults: first try user_id = getattr(request, "user_id", None); if that is None, get request_user = getattr(request, "user", None) and then set user_id = request_user.get("id") if isinstance(request_user, dict) else getattr(request_user, "id", None); update the block that references request, user_id and request_user to use these getattr defaults and remove the hasattr checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/handlers/bugs.py`:
- Around line 273-281: The code repeatedly uses hasattr/getattr to extract a
user id from request; simplify by using getattr with defaults: first try user_id
= getattr(request, "user_id", None); if that is None, get request_user =
getattr(request, "user", None) and then set user_id = request_user.get("id") if
isinstance(request_user, dict) else getattr(request_user, "id", None); update
the block that references request, user_id and request_user to use these getattr
defaults and remove the hasattr checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 69970cd7-b172-40c0-bd1d-c6b80c733a61
📒 Files selected for processing (2)
src/handlers/bugs.pytests/test_bugs_security.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_bugs_security.py
|
@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: 24965 🏁 Script executed: Length of output: 18979
PR
|
| Area | Status |
|---|---|
| Core security fix (mass-assignment) | ✅ Positive allowlist correctly blocks verified, user, closed_by, reporter_ip_address, views, is_hidden, rewarded, status |
| Server-managed defaults | ✅ views=0, verified=0, status="open", is_hidden=0, rewarded=0, closed_by=NULL are all hardcoded server-side |
| reporter_ip handling | ✅ Read from CF-Connecting-IP header only — never from the JSON body |
| URL & description validation | ✅ Type check, trim, length ≤200, http/https scheme, netloc presence |
| Test coverage (happy path + main rejections) | ✅ 6 tests cover forbidden fields, allowlist defaults, non-string/blank URL and description |
| SQL injection safety | ✅ All queries use parameterized binds |
⚠️ Issues That Should Be Addressed Before Merge
🔴 High Severity
-
No authentication enforcement on
POST /bugs
The router (src/main.py) has no JWT middleware, andbugs.pynever checks for an authenticated user before accepting a bug submission. Any anonymous internet user can create bugs. If this is intentional (public reporting), it must be explicitly documented; otherwise, JWT validation fromauth.pyshould be applied. -
domainandhuntaccept any integer without FK existence check
Both are inclient_writable_fieldsbut no query validates that the submitteddomainorhuntID actually exists. A non-existent ID will hit the DB's foreign key constraint and return a raw500 Internal Server Errorinstead of a clean400 Bad Request.
🟡 Medium Severity
-
scoreis client-writable with no range or semantic validation
A client can submit"score": -99999or"score": 9999999. The column has a DB-level integer range check (-2147483648to2147483647) but no business-logic bounds. Consider restricting to a valid range (e.g., 0–100) in the handler. -
cve_id,github_url, anduser_agentlength is not validated in the handler
The migration enforcesLENGTH(cve_id) <= 16,LENGTH(github_url) <= 200, andLENGTH(user_agent) <= 255via DBCHECKconstraints. A violation produces an unhandled500, not a descriptive400. These should be pre-validated:# Example additions to the handler cve_id = sanitized_body.get("cve_id") if cve_id is not None and len(str(cve_id)) > 16: return error_response("cve_id must be 16 characters or less", status=400) github_url = sanitized_body.get("github_url") if github_url is not None and len(str(github_url)) > 200: return error_response("github_url must be 200 characters or less", status=400)
-
Missing test coverage for several validation branches
The following cases have handler code but no tests:- URL with invalid protocol (e.g.,
ftp://example.com) → expects 400 - URL longer than 200 characters → expects 400
- URL with no valid domain (e.g.,
http://) → expects 400 domainwith a non-integer/non-existent valuecve_idexceeding 16 characters
- URL with invalid protocol (e.g.,
🟢 Minor / Code Quality
-
Redundant conditional
.get()pattern throughout the INSERT block# Current (verbose and unnecessary) sanitized_body.get("markdown_description") if "markdown_description" in sanitized_body else None # Simpler equivalent sanitized_body.get("markdown_description") # already returns None if missing
-
Commit message mentions "normalize url" but no normalization occurs
The handler strips whitespace and validates the URL, but does not normalize it (e.g., lowercase scheme/host, strip fragments like#section). If normalization is intended (as suggested by the commit message), it should be added; otherwise the commit message should be updated.
Summary
Overall merge readiness: NOT READY (needs fixes before merge)
| Category | Finding |
|---|---|
| Core security fix | ✅ Correct and well-tested |
| Authentication | 🔴 POST /bugs is fully unauthenticated — must be decided and documented |
FK validation (domain, hunt) |
🔴 Missing — causes 500 on invalid IDs |
| Input length pre-validation | 🟡 Missing for cve_id, github_url, user_agent |
score bounds |
🟡 No semantic limits |
| Test coverage gaps | 🟡 URL protocol, URL length, invalid domain format untested |
| Code cleanliness | 🟢 Minor verbosity in .get() pattern |
The core mass-assignment vulnerability fix is sound and correctly implemented. However, the PR should resolve at minimum the authentication decision (#1) and the FK existence validation (#2) before merging to avoid introducing new 500-error paths.
SatishKumar620
left a comment
SatishKumar620
left a comment
There was a problem hiding this comment.
Add authentication enforcement on POST /bugs and fix other issues
|
@coderabbitai please review the PR strictly within its intended scope (fixing the mass-assignment vulnerability in the bug creation handler). |
|
Tip For best results, initiate chat on the files or code changes.
✅ Core Fix — Correct and Safe to MergeThe mass-assignment protection is correctly implemented:
🔴 One Critical Issue Within Scope
The handler now rejects a blank description with a 400. However, looking at the tests, the async def test_blank_description():
body = {"url": "https://example.com", "description": " "}
req = MockRequest(body)
result = await handle_create_bug(req, MockEnv(), None)
assert result["status"] == 400
assert "description" in result["body"]["error"].lower()Out-of-Scope (Pre-Existing, Not Introduced by This PR)The following issues exist in the codebase independently of this change and should not block this PR's merge:
These are valid follow-up items but are pre-existing concerns. Verdict: The mass-assignment fix is correct and safe. The one item worth addressing before merge is adding a test for the newly-introduced |
|
@coderabbitai I think this may be a misread — blank description validation is already covered by test_create_bug_rejects_blank_description, which exercises the API path. Please let me know if there are any remaining blockers strictly within the scope of the mass-assignment fix. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 341
Scoped verdict (mass-assignment fix only): There are no remaining blockers within the scope of this PR. The fix is correct and complete:
The PR is safe to merge from a mass-assignment fix perspective. |
yashhzd
left a comment
yashhzd
left a comment
There was a problem hiding this comment.
Good work on this security fix. Reviewed the diff, tests, and all CodeRabbit feedback threads — the latest revision addresses every actionable comment.
What's done well:
- Positive allowlist approach (
client_writable_fieldsset) is the correct pattern for mass-assignment prevention — much stronger than a blocklist - Server-managed fields (
views=0,verified=0,status="open",is_hidden=0,rewarded=0) are now explicitly hardcoded in the INSERT, not derived from user input CF-Connecting-IPis the correct Cloudflare header for real client IP- The
if "key" in sanitized_body else Nonepattern correctly preserves falsy values likescore=0(the earlieror Nonepattern was a bug — addressed in latest commit) - URL and description validation now checks type, strips whitespace, and rejects blanks before hitting the DB
- 263 lines of focused security tests covering forbidden fields, allowlist enforcement, type validation, and blank input rejection
Minor observations (non-blocking):
- The
user_agentfield is in the client allowlist, but you could also auto-capture it fromrequest.headers.get("User-Agent")similar to howreporter_ipis extracted fromCF-Connecting-IP— this would prevent spoofing. Not required for this PR though. - The forward-compatible
request.user_id/request.userresolution pattern is a nice touch for future auth middleware integration. - Re: SatishKumar620's request for auth enforcement — I'd consider that a separate concern. This PR's scope (input sanitization + server defaults) is correct and self-contained. Auth middleware is a larger architectural change that should be its own PR.
The INSERT still uses raw SQL rather than the project's ORM (Bug.create()). Given DonnieBLT's preference for ORM usage (noted on other PRs), this might come up during maintainer review — but the security fix itself is solid and shouldn't be blocked on that refactor.
LGTM — approving.
owasp-blt
bot
added
has-peer-review
|
@DonnieBLT, Could you please check this? Thanks! |
3 participants
Summary
This PR fixes a security issue in POST /bugs where clients could set
server-managed fields directly via the request body.
Affected fields:
Requests containing these fields are now rejected with HTTP 400 and the
values are controlled exclusively by the server.
A regression test has been added to ensure these fields cannot be set
via the API.
Summary by CodeRabbit
Bug Fixes
Tests