Fix IDOR in post/comment deletion, add admin authorization, disable debug by default#252
Fix IDOR in post/comment deletion, add admin authorization, disable debug by default#252saaa99999999 wants to merge 2 commits into
Conversation
- Remove hardcoded SMTP credentials (CWE-798) - Require DEFAULT_ADMIN_PASSWORD from environment (CWE-1392) - Fix missing authorization check in admin panel (CWE-862) - Fix open redirect in login route (CWE-601) Co-Authored-By: Security Researcher <noreply@anthropic.com>
…sable debug by default - CWE-639: Add ownership verification to delete_post() and delete_comment() to prevent unauthorized deletion by non-owners - CWE-862: Add admin role check to /admin/posts and /admin/comments endpoints - CWE-489: Change DEBUG_MODE default from True to False to prevent production information disclosure via Werkzeug debugger Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR hardens authorization and security across the application. Admin routes now verify user roles before rendering dashboards, delete utilities enforce role or ownership authorization and return boolean results, login redirects are validated against unsafe schemes, and hardcoded SMTP credentials are removed from settings with required configuration enforcement. ChangesAuthorization and security hardening across routes and utilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/routes/admin_panel_users.py (1)
20-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
session["userName"]for auth-state checks and lookups.Lines [20]-[23] use
usernamesession key; switch touserNameto match the authentication contract.As per coding guidelines, "Session-based authentication must check `session["userName"]` for user authentication state".Suggested patch
- if "username" in session: - Log.info(f"Admin: {session['username']} reached to users admin panel") + if "userName" in session: + Log.info(f"Admin: {session['userName']} reached to users admin panel") @@ - user = User.query.filter_by(username=session["username"]).first() + user = User.query.filter_by(username=session["userName"]).first() @@ - f"Admin: {session['username']} deleted user: {request.form['username']}" + f"Admin: {session['userName']} deleted user: {request.form['username']}" @@ - f"Admin: {session['username']} changed {request.form['username']}'s role" + f"Admin: {session['userName']} changed {request.form['username']}'s role"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/routes/admin_panel_users.py` around lines 20 - 23, Replace uses of the session key "username" with "userName" to follow the auth contract: update the Log.info call and the lookup in User.query.filter_by to use session["userName"] (e.g., change Log.info(f"Admin: {session['username']} ...") and User.query.filter_by(username=session["username"]).first() to reference session["userName"]); keep the DB filter field name (username) as-is if the model column is still named username.app/routes/dashboard.py (1)
27-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHonor
delete_post()'s boolean result.This still redirects unconditionally, so unauthorized or missing deletes follow the same path as successful ones. That defeats the new
boolcontract and hides delete failures from the caller.Suggested fix
if "post_delete_button" in request.form: - delete_post(request.form["post_id"], session.get("username")) - - return ( - redirect(url_for("dashboard.dashboard", username=username)), - 301, - ) + if delete_post(request.form["post_id"], session.get("username")): + return ( + redirect(url_for("dashboard.dashboard", username=username)), + 301, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/routes/dashboard.py` around lines 27 - 33, The code currently ignores delete_post(...)'s boolean return and always redirects; change the block to capture the result of delete_post(request.form["post_id"], session.get("username")) and only perform redirect(url_for("dashboard.dashboard", username=username)), 301 when that result is True; if False, return an appropriate failure response (e.g., abort(403) or re-render the dashboard with an error/flash message) so unauthorized or failed deletes are not treated as successful.
🧹 Nitpick comments (1)
app/routes/admin_panel_users.py (1)
49-81: ⚡ Quick winRemove the now-redundant second admin-role branch.
After the early return at Line [28]-Line [33],
if user.role == "admin"at Line [49] is always true and theelsepath is dead code.Suggested patch
- if user.role == "admin": - query = User.query - users_objects, page, total_pages = paginate_query(query) - - users = [ - ( - u.user_id, - u.username, - u.email, - u.password, - u.profile_picture, - u.role, - u.points, - u.time_stamp, - u.is_verified, - ) - for u in users_objects - ] - - Log.info(f"Rendering admin_panel_users.html: params: users={len(users)}") - - return render_template( - "admin_panel_users.html", - users=users, - page=page, - total_pages=total_pages, - ) - else: - Log.error( - f"{request.remote_addr} tried to reach user admin panel without being admin" - ) - - return redirect("/") + query = User.query + users_objects, page, total_pages = paginate_query(query) + + users = [ + ( + u.user_id, + u.username, + u.email, + u.password, + u.profile_picture, + u.role, + u.points, + u.time_stamp, + u.is_verified, + ) + for u in users_objects + ] + + Log.info(f"Rendering admin_panel_users.html: params: users={len(users)}") + + return render_template( + "admin_panel_users.html", + users=users, + page=page, + total_pages=total_pages, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/routes/admin_panel_users.py` around lines 49 - 81, The second "if user.role == 'admin'" branch is redundant because an earlier early-return guarantees the user is admin; remove that conditional and its else branch, leaving the body that builds users (using paginate_query and users_objects), logs via Log.info, and returns render_template("admin_panel_users.html", users=users, page=page, total_pages=total_pages); also remove the dead Log.error and redirect("/") else path (references: user.role, paginate_query, users_objects, Log.info, render_template, request.remote_addr).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/routes/admin_panel_comments.py`:
- Around line 19-25: The route is checking session["username"] instead of the
mandated session["userName"], causing auth drift; update the authentication
lookup in the admin comments route to use session["userName"] (or
session.get("userName") to avoid KeyError) when querying User (the
User.query.filter_by(...) call) and basing the admin check (user.role !=
"admin") and Log.error/redirect on that value so all routes consistently use
session["userName"] for auth state.
In `@app/routes/admin_panel_posts.py`:
- Around line 19-25: The authentication check is using the wrong session key;
change the lookup from session["username"] to session["userName"] so the auth
contract is honored: update the call that builds `user =
User.query.filter_by(username=... ).first()` to read the username from
session["userName"] and keep the subsequent admin-role check and
Log.error/redirect behavior unchanged (refer to the `user` variable and the
session access in this route handler in admin_panel_posts.py).
In `@app/routes/login.py`:
- Around line 37-42: The redirect validation around the direct variable that
uses urlparse (the block checking parsed.netloc or parsed.scheme or not
direct.startswith("/")) is bypassable by inputs like "///evil.com" and must be
hardened; replace this ad-hoc check with Django's safe URL validation by calling
url_has_allowed_host_and_scheme(direct, allowed_hosts={...}) or validate against
a strict allowlist of allowed redirect paths before calling redirect(direct),
and ensure any before/after transformations (e.g., direct.replace("&", "/"))
happen prior to validation so redirect(direct) only executes for URLs that pass
url_has_allowed_host_and_scheme or the allowlist check.
In `@app/routes/post.py`:
- Around line 45-51: The handler currently only returns on successful
delete_post(post.id, session.get("username")) or delete_comment(...), so when
those helpers return False it falls through and later tries to read
request.form["comment"] causing a BadRequestKeyError; update the branch handling
for delete_post and delete_comment to explicitly return a failure response
(e.g., abort(403) or redirect with an error flash) when the helper returns False
instead of falling through — modify the blocks around delete_post and
delete_comment so they check the boolean result and immediately return an
appropriate response (use delete_post, delete_comment, request.form["comment"],
and the post view/url_id to locate the spots to change).
In `@app/utils/delete.py`:
- Around line 41-43: The admin check uses
User.query.filter_by(username=username) which is case-sensitive and mismatches
the case-insensitive is_author logic; fix by performing a case-insensitive
lookup (e.g., use SQLAlchemy func.lower(User.username) == username.lower() or
User.username.ilike(username)) when resolving User for is_admin, and apply the
same change to the other occurrence that sets is_admin (the block that mirrors
lines 120-122). Ensure you guard for None username the same way as is_author and
keep comparisons using post.author.lower() for consistency.
---
Outside diff comments:
In `@app/routes/admin_panel_users.py`:
- Around line 20-23: Replace uses of the session key "username" with "userName"
to follow the auth contract: update the Log.info call and the lookup in
User.query.filter_by to use session["userName"] (e.g., change Log.info(f"Admin:
{session['username']} ...") and
User.query.filter_by(username=session["username"]).first() to reference
session["userName"]); keep the DB filter field name (username) as-is if the
model column is still named username.
In `@app/routes/dashboard.py`:
- Around line 27-33: The code currently ignores delete_post(...)'s boolean
return and always redirects; change the block to capture the result of
delete_post(request.form["post_id"], session.get("username")) and only perform
redirect(url_for("dashboard.dashboard", username=username)), 301 when that
result is True; if False, return an appropriate failure response (e.g.,
abort(403) or re-render the dashboard with an error/flash message) so
unauthorized or failed deletes are not treated as successful.
---
Nitpick comments:
In `@app/routes/admin_panel_users.py`:
- Around line 49-81: The second "if user.role == 'admin'" branch is redundant
because an earlier early-return guarantees the user is admin; remove that
conditional and its else branch, leaving the body that builds users (using
paginate_query and users_objects), logs via Log.info, and returns
render_template("admin_panel_users.html", users=users, page=page,
total_pages=total_pages); also remove the dead Log.error and redirect("/") else
path (references: user.role, paginate_query, users_objects, Log.info,
render_template, request.remote_addr).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bcbc66c-ebea-49f5-9b8f-05519f3b7843
📒 Files selected for processing (8)
app/routes/admin_panel_comments.pyapp/routes/admin_panel_posts.pyapp/routes/admin_panel_users.pyapp/routes/dashboard.pyapp/routes/login.pyapp/routes/post.pyapp/settings.pyapp/utils/delete.py
| user = User.query.filter_by(username=session["username"]).first() | ||
|
|
||
| if not user or user.role != "admin": | ||
| Log.error( | ||
| f"{request.remote_addr} tried to reach comment admin panel without being admin" | ||
| ) | ||
| return redirect("/") |
There was a problem hiding this comment.
Use the required session["userName"] key for authentication state.
Line [19] uses session["username"]; this route should read auth state via session["userName"] consistently to avoid authorization drift across routes.
Suggested patch
- if "username" in session:
- user = User.query.filter_by(username=session["username"]).first()
+ if "userName" in session:
+ user = User.query.filter_by(username=session["userName"]).first()
@@
- Log.info(f"Admin: {session['username']} reached to comments admin panel")
+ Log.info(f"Admin: {session['userName']} reached to comments admin panel")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/admin_panel_comments.py` around lines 19 - 25, The route is
checking session["username"] instead of the mandated session["userName"],
causing auth drift; update the authentication lookup in the admin comments route
to use session["userName"] (or session.get("userName") to avoid KeyError) when
querying User (the User.query.filter_by(...) call) and basing the admin check
(user.role != "admin") and Log.error/redirect on that value so all routes
consistently use session["userName"] for auth state.
| user = User.query.filter_by(username=session["username"]).first() | ||
|
|
||
| if not user or user.role != "admin": | ||
| Log.error( | ||
| f"{request.remote_addr} tried to reach post admin panel without being admin" | ||
| ) | ||
| return redirect("/") |
There was a problem hiding this comment.
Align session auth key to session["userName"].
Line [19] reads session["username"]; this should use session["userName"] for the authentication-state contract.
Suggested patch
- if "username" in session:
- user = User.query.filter_by(username=session["username"]).first()
+ if "userName" in session:
+ user = User.query.filter_by(username=session["userName"]).first()
@@
- Log.info(f"Admin: {session['username']} reached to posts admin panel")
+ Log.info(f"Admin: {session['userName']} reached to posts admin panel")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/admin_panel_posts.py` around lines 19 - 25, The authentication
check is using the wrong session key; change the lookup from session["username"]
to session["userName"] so the auth contract is honored: update the call that
builds `user = User.query.filter_by(username=... ).first()` to read the username
from session["userName"] and keep the subsequent admin-role check and
Log.error/redirect behavior unchanged (refer to the `user` variable and the
session access in this route handler in admin_panel_posts.py).
| if direct: | ||
| parsed = urlparse(direct) | ||
| if parsed.netloc or parsed.scheme or not direct.startswith("/"): | ||
| direct = "/" | ||
| else: | ||
| direct = "/" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files "**/*.py" | grep -E "(login|auth)" | head -20Repository: DogukanUrker/FlaskBlog
Length of output: 357
🏁 Script executed:
find . -name "login.py" -type fRepository: DogukanUrker/FlaskBlog
Length of output: 88
🏁 Script executed:
cat -n app/routes/login.pyRepository: DogukanUrker/FlaskBlog
Length of output: 4125
🏁 Script executed:
cd /repo && python3 << 'EOF'
from urllib.parse import urlparse
# Test cases from the review
test_cases = [
# Normal safe cases
("/dashboard", "Should be safe"),
("/", "Should be safe"),
# Payloads after & -> / replacement
("///evil.com", "After replace: &&&evil.com"),
("/\evil.com", "After replace: &\\evil.com"),
("//evil.com", "Double slash case"),
("/evil.com", "Single path-like"),
# URL with scheme
("http://evil.com", "Full URL"),
("https://evil.com", "HTTPS URL"),
]
print("Testing current validation logic:")
print("=" * 70)
for direct_value, description in test_cases:
print(f"\nInput: {repr(direct_value)} ({description})")
# Current validation code from lines 37-42
parsed = urlparse(direct_value)
is_blocked = parsed.netloc or parsed.scheme or not direct_value.startswith("/")
print(f" urlparse result:")
print(f" - scheme: {repr(parsed.scheme)}")
print(f" - netloc: {repr(parsed.netloc)}")
print(f" - path: {repr(parsed.path)}")
print(f" Validation checks:")
print(f" - has netloc: {bool(parsed.netloc)}")
print(f" - has scheme: {bool(parsed.scheme)}")
print(f" - starts with '/': {direct_value.startswith('/')}")
print(f" → BLOCKED: {is_blocked} (redirects to '/')")
EOFRepository: DogukanUrker/FlaskBlog
Length of output: 122
🏁 Script executed:
cd /repo && python3 << 'EOF'
# Test Flask redirect behavior with potentially dangerous URLs
from flask import Flask, redirect
import sys
app = Flask(__name__)
test_cases = [
("/dashboard", "Safe relative"),
("///evil.com", "Triple slash"),
("/\evil.com", "Backslash variant"),
("//evil.com", "Double slash"),
]
print("\nTesting Flask redirect() behavior:")
print("=" * 70)
with app.test_request_context():
for url_value, description in test_cases:
try:
response = redirect(url_value)
print(f"\nInput: {repr(url_value)} ({description})")
print(f" Response location: {repr(response.location)}")
print(f" Status code: {response.status_code}")
except Exception as e:
print(f"\nInput: {repr(url_value)} ({description})")
print(f" Error: {e}")
EOFRepository: DogukanUrker/FlaskBlog
Length of output: 122
🏁 Script executed:
python3 << 'EOF'
from urllib.parse import urlparse
# Test cases from the review
test_cases = [
# Normal safe cases
("/dashboard", "Should be safe"),
("/", "Should be safe"),
# Payloads after & -> / replacement
("///evil.com", "After replace: &&&evil.com"),
("/\evil.com", "After replace: &\\evil.com"),
("//evil.com", "Double slash case"),
("/evil.com", "Single path-like"),
# URL with scheme
("http://evil.com", "Full URL"),
("https://evil.com", "HTTPS URL"),
]
print("Testing current validation logic:")
print("=" * 70)
for direct_value, description in test_cases:
print(f"\nInput: {repr(direct_value)} ({description})")
# Current validation code from lines 37-42
parsed = urlparse(direct_value)
is_blocked = parsed.netloc or parsed.scheme or not direct_value.startswith("/")
print(f" urlparse result:")
print(f" - scheme: {repr(parsed.scheme)}")
print(f" - netloc: {repr(parsed.netloc)}")
print(f" - path: {repr(parsed.path)}")
print(f" Validation checks:")
print(f" - has netloc: {bool(parsed.netloc)}")
print(f" - has scheme: {bool(parsed.scheme)}")
print(f" - starts with '/': {direct_value.startswith('/')}")
print(f" → BLOCKED: {is_blocked} (redirects to '/')")
EOFRepository: DogukanUrker/FlaskBlog
Length of output: 2198
🏁 Script executed:
python3 << 'EOF'
# Test Flask redirect behavior with potentially dangerous URLs
try:
from flask import Flask, redirect
app = Flask(__name__)
test_cases = [
("/dashboard", "Safe relative"),
("///evil.com", "Triple slash"),
("/\evil.com", "Backslash variant"),
("//evil.com", "Double slash"),
]
print("\nTesting Flask redirect() behavior:")
print("=" * 70)
with app.test_request_context():
for url_value, description in test_cases:
try:
response = redirect(url_value)
print(f"\nInput: {repr(url_value)} ({description})")
print(f" Response location: {repr(response.location)}")
print(f" Status code: {response.status_code}")
except Exception as e:
print(f"\nInput: {repr(url_value)} ({description})")
print(f" Error: {e}")
except ImportError as e:
print(f"Flask not available in sandbox: {e}")
EOFRepository: DogukanUrker/FlaskBlog
Length of output: 122
🌐 Web query:
browser behavior triple slash URL redirect ///evil.com open redirect vulnerability
💡 Result:
A triple slash (///) or double slash (//) at the beginning of a URL path can lead to an open redirect vulnerability if the web application's server-side logic improperly handles or normalizes these paths [1][2][3]. The vulnerability is not caused by the browser's inherent behavior, but rather by how the application processes the URL [4][2]. When a web application receives a request like https://vulnerable-site.com///evil.com, it may interpret the leading slashes in the path as a signal to treat the remainder of the string as an absolute URL or a different domain [1][2]. If the application's redirect logic—often used for features like "next" or "return_to" parameters—fails to validate or sanitize this input, it may blindly redirect the user to the attacker-controlled domain [1][5][6]. Key aspects of this vulnerability include: 1. Path Normalization Issues: Many web frameworks or middleware components attempt to "normalize" URLs by stripping extra slashes [4][2]. If this normalization process is flawed, it can inadvertently transform a path-based request into an external redirect [2]. 2. Weak Validation: Developers often implement security checks that only verify if a URL starts with a trusted domain [1][7]. Attackers can bypass these checks using multiple slashes or URL encoding (e.g., %2F%2F) to trick the application into seeing a trusted path while the underlying server processes it as an external destination [1][7]. 3. Server-Side Processing: The vulnerability typically occurs on the server side [2][6]. While browsers generally treat multiple slashes in a path as a single slash for resource requests, the server-side application logic may parse the string differently, leading to the redirect [4][2]. To prevent this, applications should: - Avoid using user-supplied input directly in redirect functions [8][6]. - Implement strict allowlists for redirect destinations rather than relying on blocklists or simple string matching [7][3]. - Properly normalize and validate all URL inputs on the server side before processing them [8][3].
Citations:
- 1: https://undercodetesting.com/the-silent-threat-in-your-urls-how-a-simple-double-slash-can-lead-to-a-major-security-breach/
- 2: https://securitylab.github.com/advisories/GHSL-2020-199-open-redirect-slashify
- 3: https://medium.com/@anshubind89/when-one-slash-broke-the-rules-finding-an-open-redirect-on-a-major-marketplace-9d9bc7a39fbd
- 4: https://stackoverflow.com/questions/10161177/url-with-multiple-forward-slashes-does-it-break-anything
- 5: https://resources.uprootsecurity.com/Open_URL_Redirect
- 6: https://medium.com/%40loveleshgangil/open-redirection-vulnerabilities-complete-guide-to-improper-front-end-redirect-validation-2d40599d8442
- 7: https://owasp.org/www-community/attacks/open_redirect
- 8: https://www.sitelock.com/blog/prevent-url-redirect-attacks/
🌐 Web query:
Django open redirect validation triple slash URL security
💡 Result:
Django does not have a specific, built-in "triple slash" validation rule, but it protects against open redirect vulnerabilities—including those involving malformed URLs like ///evil.com—through its core URL handling and validation utilities [1][2][3]. Open redirect vulnerabilities occur when an application redirects a user to an untrusted URL provided via user input (e.g., a next query parameter) without proper validation [1][2][3]. Attackers often use malformed URLs, such as those starting with multiple slashes (e.g., ///evil.com), to bypass simple string-based checks that only look for a single leading slash [4][5][6]. Key security practices for Django include: 1. Use Built-in Redirects: Django's built-in LoginView and other standard redirect mechanisms automatically handle validation against ALLOWED_HOSTS [1]. 2. Avoid Manual Validation: Do not attempt to manually validate URLs using string operations like startswith('/') or regex, as these are easily bypassed by malformed inputs [3][6]. 3. Use url_has_allowed_host_and_scheme(): While this is an internal utility, it is the standard mechanism Django uses to safely validate redirect targets [1][7][8]. It checks that the URL is either relative or belongs to an allowed host and uses a safe scheme (e.g., https) [7][8]. 4. Understand Internal Status: Django documentation notes that url_has_allowed_host_and_scheme() is an internal API [8][9]. Developers are encouraged to use it carefully or rely on higher-level abstractions like HttpResponseRedirect where possible [8]. 5. Historical Context: Django has historically addressed vulnerabilities related to URL handling, such as reverse() generating scheme-relative URLs (starting with //), by ensuring such URLs are properly encoded (e.g., // becomes %2F/) to prevent them from being interpreted as absolute URLs pointing to different hosts [10]. To secure your application, always validate user-supplied redirect targets against a strict allowlist of hosts and ensure the URL scheme is restricted to https [1][7][6]. If you are building custom redirect logic, ensure you are using robust, standard library-based parsing rather than custom string manipulation [3][6].
Citations:
- 1: https://vibeappscanner.com/vulnerability-in/open-redirect-django
- 2: https://www.stackhawk.com/blog/django-open-redirect-guide-examples-and-prevention/
- 3: https://owasp.org/www-community/attacks/open_redirect
- 4: https://securitylab.github.com/advisories/GHSL-2020-199-open-redirect-slashify
- 5: GHSA-mqqf-5wvp-8fh8
- 6: https://oneuptime.com/blog/post/2026-01-24-fix-open-redirect-vulnerabilities/view
- 7: https://thelinuxcode.com/url-validation-in-django-practical-rules-security-boundaries-and-clean-storage/
- 8: https://forum.djangoproject.com/t/why-is-the-use-of-url-has-allowed-host-and-scheme-discouraged/35314
- 9: https://docs.djangoproject.com/en/4.2/releases/4.2
- 10: django/django@28e7658
🌐 Web query:
RFC 3986 triple slash URL interpretation scheme-relative
💡 Result:
Under RFC 3986, a triple-slash URL (e.g., http:///example.com) is not a valid URI [1][2]. The specification defines the authority component as being preceded by exactly two slashes ("//") [3][4]. When a URI contains an authority component, the path component must either be empty or begin with a single slash ("/") [4]. Consequently, a triple-slash sequence is interpreted by strict RFC 3986 parsers as an invalid structure because it does not conform to the required authority-path syntax [1][2]. Despite this, many modern web browsers and some other clients (such as those using the WHATWG URL Standard) implement "web compatibility" fixups to handle such URLs [1][2]. These implementations often treat the extra slash as part of the path or ignore it, effectively resolving the URL as if it were a standard double-slash URI [2]. This behavior is intended to improve user experience by gracefully handling common input errors, even though it deviates from the strict generic syntax defined in RFC 3986 [1][2]. It is important to distinguish this from the "file" scheme, where a triple-slash (file:///) is often used to represent a local file path (e.g., file:///path/to/file) [5]. In this context, the first two slashes denote the authority (which is empty or "localhost"), and the third slash is the beginning of the absolute path [5]. This is consistent with the generic syntax, whereas a triple-slash in an "http" URL is not [1][5].
Citations:
- 1: handling URL with http:/// (3 slashes between protocol and domain) curl/curl#791
- 2: https://stackoverflow.com/questions/22772897/why-is-http-example-org-with-triple-slash-treated-as-a-valid-url-by-firefox
- 3: https://datatracker.ietf.org/doc/html/rfc3986
- 4: https://www.tech-invite.com/y35/tinv-ietf-rfc-3986-2.html
- 5: https://superuser.com/questions/352133/why-do-file-urls-start-with-3-slashes
The redirect validation logic allows /// payloads to bypass the check and should be hardened.
The current validation at lines 37-42 has a vulnerability: after direct.replace("&", "/"), payloads like &&&evil.com become ///evil.com, which Python's urlparse() treats as a path (not a netloc) and thus passes all three checks (parsed.netloc or parsed.scheme or not direct.startswith("/")). This allows the malformed URL to reach redirect(direct) and can lead to an open redirect. Django's documentation explicitly warns against simple string validation for this reason and recommends using url_has_allowed_host_and_scheme() or a strict allowlist instead.
🔐 Suggested hardening
direct = direct.replace("&", "/")
if direct:
- parsed = urlparse(direct)
- if parsed.netloc or parsed.scheme or not direct.startswith("/"):
+ normalized = direct.strip().replace("\\", "/")
+ parsed = urlparse(normalized)
+ if (
+ normalized.startswith("///")
+ or parsed.netloc
+ or parsed.scheme
+ or not normalized.startswith("/")
+ ):
direct = "/"
+ else:
+ direct = normalized
else:
direct = "/"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/login.py` around lines 37 - 42, The redirect validation around the
direct variable that uses urlparse (the block checking parsed.netloc or
parsed.scheme or not direct.startswith("/")) is bypassable by inputs like
"///evil.com" and must be hardened; replace this ad-hoc check with Django's safe
URL validation by calling url_has_allowed_host_and_scheme(direct,
allowed_hosts={...}) or validate against a strict allowlist of allowed redirect
paths before calling redirect(direct), and ensure any before/after
transformations (e.g., direct.replace("&", "/")) happen prior to validation so
redirect(direct) only executes for URLs that pass
url_has_allowed_host_and_scheme or the allowlist check.
| if "post_delete_button" in request.form: | ||
| delete_post(post.id) | ||
| return redirect("/") | ||
| if delete_post(post.id, session.get("username")): | ||
| return redirect("/") | ||
|
|
||
| if "comment_delete_button" in request.form: | ||
| delete_comment(request.form["comment_id"]) | ||
| return redirect(url_for("post.post", url_id=url_id)), 301 | ||
| if delete_comment(request.form["comment_id"], session.get("username")): | ||
| return redirect(url_for("post.post", url_id=url_id)), 301 |
There was a problem hiding this comment.
Return on failed delete requests.
If either delete helper returns False, this handler falls through into the comment-creation path and reads request.form["comment"]. A delete form usually won't send that field, so unauthorized/not-found deletes turn into a BadRequestKeyError instead of a clean failure response.
Suggested fix
if request.method == "POST":
if "post_delete_button" in request.form:
if delete_post(post.id, session.get("username")):
return redirect("/")
+ return redirect(url_for("post.post", url_id=url_id, slug=post_slug)), 303
if "comment_delete_button" in request.form:
if delete_comment(request.form["comment_id"], session.get("username")):
- return redirect(url_for("post.post", url_id=url_id)), 301
+ return redirect(
+ url_for("post.post", url_id=url_id, slug=post_slug)
+ ), 303
+ return redirect(
+ url_for("post.post", url_id=url_id, slug=post_slug)
+ ), 303🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/post.py` around lines 45 - 51, The handler currently only returns
on successful delete_post(post.id, session.get("username")) or
delete_comment(...), so when those helpers return False it falls through and
later tries to read request.form["comment"] causing a BadRequestKeyError; update
the branch handling for delete_post and delete_comment to explicitly return a
failure response (e.g., abort(403) or redirect with an error flash) when the
helper returns False instead of falling through — modify the blocks around
delete_post and delete_comment so they check the boolean result and immediately
return an appropriate response (use delete_post, delete_comment,
request.form["comment"], and the post view/url_id to locate the spots to
change).
| user = User.query.filter_by(username=username).first() if username else None | ||
| is_admin = user and user.role == "admin" | ||
| is_author = username and post.author.lower() == username.lower() |
There was a problem hiding this comment.
Normalize the admin lookup.
is_author is case-insensitive, but the admin lookup still uses filter_by(username=username). If the session casing differs from the stored username, admins will be denied deletes even though the same module already treats usernames case-insensitively elsewhere.
Suggested fix
+from sqlalchemy import func
+
from database import db
from models import Comment, Post, User
from utils.flash_message import flash_message
from utils.log import Log
@@
- user = User.query.filter_by(username=username).first() if username else None
+ user = (
+ User.query.filter(func.lower(User.username) == username.lower()).first()
+ if username
+ else None
+ )
@@
- user = User.query.filter_by(username=username).first() if username else None
+ user = (
+ User.query.filter(func.lower(User.username) == username.lower()).first()
+ if username
+ else None
+ )Also applies to: 120-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/utils/delete.py` around lines 41 - 43, The admin check uses
User.query.filter_by(username=username) which is case-sensitive and mismatches
the case-insensitive is_author logic; fix by performing a case-insensitive
lookup (e.g., use SQLAlchemy func.lower(User.username) == username.lower() or
User.username.ilike(username)) when resolving User for is_admin, and apply the
same change to the other occurrence that sets is_admin (the block that mirrors
lines 120-122). Ensure you guard for None username the same way as is_author and
keep comparisons using post.author.lower() for consistency.
CVE Request — Action Needed from MaintainerThis PR fixes security vulnerabilities. To assign a CVE number: GitHub only issues CVEs from the official upstream repository, not from forks. Please:
If you prefer, I can submit the CVE via MITRE (cveform.mitre.org) instead — just let me know. Thank you for reviewing this PR! |
1 participant
Summary
Fixes four security vulnerabilities in FlaskBlog:
1. CWE-639: IDOR - Unauthorized Post Deletion (Critical)
delete_post()inapp/utils/delete.pyhad no authorization check. Any authenticated user could delete any post by sending a crafted POST request to/post/<url_id>or/dashboard/<username>with apost_delete_buttonparameter.Fix: Added ownership verification (post.author == username) and admin role check to delete_post().
2. CWE-639: IDOR - Unauthorized Comment Deletion (Medium)
Same pattern in
delete_comment(). Any user could delete any comment.Fix: Added author/admin check to delete_comment().
3. CWE-862: Missing Authorization on Admin Endpoints (Medium)
/admin/postsand/admin/commentsonly checked for login status but did not verify admin role, unlike/adminand/admin/users.Fix: Added user.role == "admin" check to both routes.
4. CWE-489: Debug Mode Enabled by Default (Medium)
DEBUG_MODEdefaulted to True in settings.py, exposing Werkzeug debugger in production.Fix: Changed default to False.
Files Changed
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements