Test/sast autofix validation py#35
Conversation
📝 WalkthroughWalkthroughThis PR introduces three new HTTP endpoints for user data access and draft preview functionality. A Flask app exposes ChangesUser Query and Authentication Endpoints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| conn = get_db() | ||
| cursor = conn.cursor() | ||
|
|
||
| query = "SELECT id, name, email, role FROM users WHERE name LIKE '%" + name + "%' AND role = '" + role + "'" |
There was a problem hiding this comment.
Potential SQL injection via string-based query concatenation - critical severity
SQL injection might be possible in these locations, especially if the strings being concatenated are controlled via user input.
Show fix
Remediation: If possible, rebuild the query to use prepared statements or an ORM. If that is not possible, make sure the user input is verified or sanitized. As an added layer of protection, we also recommend installing a WAF that blocks SQL injection attacks.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| // Line 10 - helper that builds the vulnerable query | ||
| function buildUserQuery(userId) { | ||
| // Vulnerable: string interpolation of untrusted input | ||
| return `SELECT * FROM users WHERE id = ${userId}`; |
There was a problem hiding this comment.
Potential SQL injection via string-based query concatenation - critical severity
SQL injection might be possible in these locations, especially if the strings being concatenated are controlled via user input.
Show fix
Remediation: If possible, rebuild the query to use prepared statements or an ORM. Alternatively, use parseInt() to sanitize variables. If that is not possible, make sure the user input is verified or sanitized in another way. To autofix all SQL injection instances in your entire app, install Zen for Node.js.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: this is a test reason
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
this is a test reason
There was a problem hiding this comment.
🔴 We were not able to ignore this issue because of the following reason:
Only members of this Aikido workspace can ignore issues.
|
|
||
| // Line ~50 – main Express route where SAST reports the issue | ||
| async function registerUserRoutes(app, db) { | ||
| app.get('/user', async (req, res) => { |
There was a problem hiding this comment.
The anonymous async route handler passed to app.get is large and contains business logic; extract or give it a descriptive name and document it to improve readability and testability.
Details
✨ AI Reasoning
The PR adds a new Express route where the callback passed to app.get is an anonymous async arrow function that performs request logging, metric tracking, input validation, builds a SQL query, awaits a DB call, handles success and multiple error paths, and serializes output. This is non-trivial business logic spanning many lines; placing it inline hides intent and makes unit testing harder. The surrounding named function (registerUserRoutes) does not document the inline handler's behavior sufficiently, so extracting or naming it would improve clarity and maintainability.
🔧 How do I fix it?
Extract complex anonymous functions into named functions with descriptive names, or add explanatory comments for their purpose.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
no no no no
|
|
||
| const PREVIEW_SECRET = "preview"; | ||
|
|
||
| export async function GET(req: NextRequest) { | ||
| const secret = req.nextUrl.searchParams.get("secret"); | ||
|
|
||
| if (secret !== PREVIEW_SECRET) { | ||
| return NextResponse.json({ error: "Invalid token" }, { status: 401 }); | ||
| } |
There was a problem hiding this comment.
A timing attack might allow hackers to bruteforce passwords - high severity
An insecure way to compare passwords to user input might allow hackers to bruteforce passwords.
Show fix
| const PREVIEW_SECRET = "preview"; | |
| export async function GET(req: NextRequest) { | |
| const secret = req.nextUrl.searchParams.get("secret"); | |
| if (secret !== PREVIEW_SECRET) { | |
| return NextResponse.json({ error: "Invalid token" }, { status: 401 }); | |
| } | |
| import crypto from "node:crypto"; | |
| const PREVIEW_SECRET = "preview"; | |
| export async function GET(req: NextRequest) { | |
| const secret = req.nextUrl.searchParams.get("secret"); | |
| if (!secret || !crypto.timingSafeEqual(crypto.createHash('sha256').update(secret).digest(), crypto.createHash('sha256').update(PREVIEW_SECRET).digest())) { | |
| return NextResponse.json({ error: "Invalid token" }, { status: 401 }); | |
| } |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: this is a test reason number two
There was a problem hiding this comment.
🔴 We were not able to ignore this issue because of the following reason:
Only members of this Aikido workspace can ignore issues.
There was a problem hiding this comment.
🔴 We were not able to ignore this issue because of the following reason:
Only members of this Aikido workspace can ignore issues.
| const slug = slugFromParams | ||
| ? Array.isArray(slugFromParams) | ||
| ? slugFromParams[0] | ||
| : slugFromParams | ||
| : ""; |
There was a problem hiding this comment.
Nested ternary used to compute slug (ternary inside ternary). Break into clear if/else or separate assignments for readability.
Show fix
| const slug = slugFromParams | |
| ? Array.isArray(slugFromParams) | |
| ? slugFromParams[0] | |
| : slugFromParams | |
| : ""; | |
| let slug = ""; | |
| if (slugFromParams) { | |
| if (Array.isArray(slugFromParams)) { | |
| slug = slugFromParams[0]; | |
| } else { | |
| slug = slugFromParams; | |
| } | |
| } |
Details
✨ AI Reasoning
A single-line expression was introduced that nests a ternary operator inside another ternary, increasing cognitive load when determining the assigned slug value. Nested inline conditionals make the expression harder to read and maintain, especially when parsing request params and fallbacks.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| function sanitizeOutput(user) { | ||
| // pretend this is doing something complex |
There was a problem hiding this comment.
Comment rephrases implementation ('pretend this is doing something complex') without explaining intent. Replace with why the sanitization is necessary or remove it.
| function sanitizeOutput(user) { | |
| // pretend this is doing something complex | |
| function sanitizeOutput(user) { | |
| // Remove password field to prevent exposure in API responses |
Details
✨ AI Reasoning
A comment that says the code 'pretend this is doing something complex' conveys no design intent or reason; it merely paraphrases the implementation and can become misleading if code changes.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/jarno.ts (1)
44-47: ⚡ Quick winValidate the shape of
userIdbefore using it.
req.query.userIdis an untrusted string (or array, if the same key is repeated in the query string — Express returns an array in that case). Even with parameterized queries, accepting arbitrary shapes can produce confusing 500s from the DB layer. Consider coercing to the expected type (e.g., integer) and rejecting otherwise with400.♻️ Proposed validation
- const userId = req.query.userId; - if (!userId) { - return badRequest(res, 'Missing userId'); - } + const rawUserId = req.query.userId; + if (typeof rawUserId !== 'string' || rawUserId.length === 0) { + return badRequest(res, 'Missing or invalid userId'); + } + const userId = Number(rawUserId); + if (!Number.isInteger(userId) || userId <= 0) { + return badRequest(res, 'userId must be a positive integer'); + }🤖 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 `@src/jarno.ts` around lines 44 - 47, The code currently trusts req.query.userId directly; change the validation in src/jarno.ts around the userId handling so you first ensure req.query.userId is a single string (not an array) and then coerce/validate it to the expected type (e.g., integer) before using it; if req.query.userId is an array or fails the integer check (use parseInt or a /^\d+$/ check and Number.isFinite/Number.isInteger after coercion) return badRequest(res, 'Invalid userId') instead of proceeding, and use the same userId variable name so later code that consumes userId continues to work.scripts/user_lookup.py (1)
18-27: ⚡ Quick winConnection leaks on exception paths.
If
cursor.execute(query)raises (orfetchall()does), theconn.close()on Line 25 is skipped and the SQLite connection is leaked. Over time this can lead toOperationalError: database is lockedor file-descriptor exhaustion for the Flask process. Note that thesqlite3.connect(…)context manager will hold connections open, so you need to remember to close it manually or write your own context manager — a barewith sqlite3.connect(...)does not close the connection, it only manages the transaction.Use
try/finallyor wrap the connection incontextlib.closing.♻️ Proposed fix using `contextlib.closing`
+from contextlib import closing @@ - conn = get_db() - cursor = conn.cursor() - - query = "SELECT id, name, email, role FROM users WHERE name LIKE '%" + name + "%' AND role = '" + role + "'" - cursor.execute(query) - - rows = cursor.fetchall() - conn.close() + with closing(get_db()) as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT id, name, email, role FROM users WHERE name LIKE ? AND role = ?", + (f"%{name}%", role), + ) + rows = cursor.fetchall() return jsonify([dict(r) for r in rows])🤖 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 `@scripts/user_lookup.py` around lines 18 - 27, The DB connection in scripts/user_lookup.py (created via get_db() and used by cursor.execute/fetchall) can be leaked if execute or fetchall raises; wrap the connection and cursor in contextlib.closing (or a try/finally) so conn.close() is always called: import contextlib, use "with contextlib.closing(get_db()) as conn:" and "with contextlib.closing(conn.cursor()) as cursor:" around the query/execute/fetchall and then return jsonify([dict(r) for r in rows]); ensure you remove the bare conn.close() and instead let the context managers guarantee closure even on exceptions.
🤖 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 `@scripts/user_lookup.py`:
- Around line 30-31: The script currently calls app.run(host="0.0.0.0",
port=5000) in the __main__ block which exposes the dev server on all interfaces;
change this to read the bind address from configuration (e.g., an environment
variable) with a safe default of "127.0.0.1" and only allow "0.0.0.0" when
explicitly configured, and update any documentation/comments to recommend using
a production WSGI server (gunicorn/uWSGI) instead of Flask.run for non-local
deployments; locate the app.run call in the __main__ guard to implement this
change.
- Around line 21-22: The SQL query in scripts/user_lookup.py currently builds a
string using name and role which allows SQL injection; modify the code that
creates and executes the query so it uses DB-API parameter binding (sqlite3 '?'
placeholders) instead of concatenation: build a wildcard pattern for the LIKE
(e.g., pattern = "%" + name + "%") and pass (pattern, role) as parameters to
cursor.execute rather than interpolating into query; optionally escape '%' and
'_' in name and add an ESCAPE '\' clause to the LIKE if you need literal
percent/underscore handling.
In `@src/jarno.ts`:
- Around line 4-7: buildUserQuery currently constructs SQL by interpolating the
untrusted userId (SQL injection); change it to return a parameterized query plus
values (or change its callers) instead of string concatenation: update
buildUserQuery to produce a query string with the proper placeholder syntax for
your driver (e.g. $1 for pg or ? for mysql/sqlite) and return the values array
(or have it return both parts), then update the db.query call site (the place
that currently calls db.query(query)) to call db.query(queryText, values) so the
driver binds parameters safely; ensure you pick the exact placeholder style your
DB client uses and remove any direct interpolation of user input in
buildUserQuery.
In `@src/sast.ts`:
- Around line 32-34: The handler currently returns a 401 when pageData is
missing; change this to a resource error (use 404 for not found, or 400 if you
consider it malformed) by updating the NextResponse.json call that checks
pageData so it returns status: 404 (and adjust the message if desired). Locate
the if (!pageData) branch in src/sast.ts where NextResponse.json is called and
replace the 401 status with 404 to reflect a non-authentication "Invalid slug"
response.
- Around line 7-14: The GET handler uses a hardcoded PREVIEW_SECRET string;
replace this with an environment-backed secret (e.g.,
process.env.PREVIEW_SECRET) and stop using the literal "preview". Update the
module to read the secret once (const PREVIEW_SECRET =
process.env.PREVIEW_SECRET) and ensure the GET function checks that
PREVIEW_SECRET is present (return a 500 or fail-fast if missing) before
comparing it to req.nextUrl.searchParams.get("secret"); avoid defaulting to a
weak fallback and consider using a constant-time comparison if available for
security.
- Around line 23-37: The redirect uses untrusted locale and slug directly which
can enable unsafe or manipulated paths; sanitize and validate both values before
calling redirect(): ensure locale is one of allowed values (compare locale
against a whitelist or DEFAULT_LOCALE) and normalize slug to a safe path segment
(reject or percent-encode any input containing //, backslashes, control chars,
or host-like patterns, and strip leading/trailing slashes), then call
draftMode().enable() and redirect() with the validated/normalized values (update
the code around locale, slug, getPagesData, draftMode(), and redirect() to use
the sanitized variables).
---
Nitpick comments:
In `@scripts/user_lookup.py`:
- Around line 18-27: The DB connection in scripts/user_lookup.py (created via
get_db() and used by cursor.execute/fetchall) can be leaked if execute or
fetchall raises; wrap the connection and cursor in contextlib.closing (or a
try/finally) so conn.close() is always called: import contextlib, use "with
contextlib.closing(get_db()) as conn:" and "with
contextlib.closing(conn.cursor()) as cursor:" around the query/execute/fetchall
and then return jsonify([dict(r) for r in rows]); ensure you remove the bare
conn.close() and instead let the context managers guarantee closure even on
exceptions.
In `@src/jarno.ts`:
- Around line 44-47: The code currently trusts req.query.userId directly; change
the validation in src/jarno.ts around the userId handling so you first ensure
req.query.userId is a single string (not an array) and then coerce/validate it
to the expected type (e.g., integer) before using it; if req.query.userId is an
array or fails the integer check (use parseInt or a /^\d+$/ check and
Number.isFinite/Number.isInteger after coercion) return badRequest(res, 'Invalid
userId') instead of proceeding, and use the same userId variable name so later
code that consumes userId continues to work.
🪄 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: 12d47707-74e7-42fa-99e5-c0e8a5fe99e4
📒 Files selected for processing (3)
scripts/user_lookup.pysrc/jarno.tssrc/sast.ts
| query = "SELECT id, name, email, role FROM users WHERE name LIKE '%" + name + "%' AND role = '" + role + "'" | ||
| cursor.execute(query) |
There was a problem hiding this comment.
Critical: SQL injection via string concatenation of name and role.
The query at Line 21 concatenates request.args.get("name") and request.args.get("role") directly into SQL, so ?name=%' OR '1'='1 (or a stacked statement / UNION SELECT password ...) lets a caller exfiltrate arbitrary rows from the users table. This is the same anti-pattern flagged in src/jarno.ts. Both Ruff (S608) and SonarCloud have already flagged this.
Use the DB-API placeholder syntax (? for sqlite3) and pass the values as bound parameters. For LIKE, build the wildcard pattern in Python and bind the whole pattern as a single parameter.
🔒 Proposed parameterized fix
- query = "SELECT id, name, email, role FROM users WHERE name LIKE '%" + name + "%' AND role = '" + role + "'"
- cursor.execute(query)
+ query = "SELECT id, name, email, role FROM users WHERE name LIKE ? AND role = ?"
+ cursor.execute(query, (f"%{name}%", role))Additionally, if name may legitimately contain % or _, consider escaping those LIKE metacharacters before binding (e.g., via name.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_") and an ESCAPE '\' clause), so users can't expand the result set with wildcards.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 22-22: Change this code to not construct SQL queries directly from user-controlled data.
🪛 Ruff (0.15.12)
[error] 21-21: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@scripts/user_lookup.py` around lines 21 - 22, The SQL query in
scripts/user_lookup.py currently builds a string using name and role which
allows SQL injection; modify the code that creates and executes the query so it
uses DB-API parameter binding (sqlite3 '?' placeholders) instead of
concatenation: build a wildcard pattern for the LIKE (e.g., pattern = "%" + name
+ "%") and pass (pattern, role) as parameters to cursor.execute rather than
interpolating into query; optionally escape '%' and '_' in name and add an
ESCAPE '\' clause to the LIKE if you need literal percent/underscore handling.
| if __name__ == "__main__": | ||
| app.run(host="0.0.0.0", port=5000) |
There was a problem hiding this comment.
Avoid binding to 0.0.0.0 by default; gate it behind config.
app.run(host="0.0.0.0", ...) exposes the dev server on every network interface — flagged by Ruff (S104), ast-grep, and SonarCloud. Combined with the SQL-injection issue above, this materially raises blast radius if the script is ever launched on a machine reachable from an untrusted network. Additionally, Flask.run is the Werkzeug dev server and is not intended for production.
Default the bind address to 127.0.0.1 (or read it from an env var), and use a production WSGI server (gunicorn/uWSGI) for any non-local deployment.
🛡️ Proposed change
-if __name__ == "__main__":
- app.run(host="0.0.0.0", port=5000)
+if __name__ == "__main__":
+ import os
+ host = os.environ.get("APP_HOST", "127.0.0.1")
+ port = int(os.environ.get("APP_PORT", "5000"))
+ app.run(host=host, port=port)🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 30-30: Running flask app with host 0.0.0.0 could expose the server publicly.
Context: app.run(host="0.0.0.0", port=5000)
Note: [CWE-668]: Exposure of Resource to Wrong Sphere [OWASP A01:2021]: Broken Access Control [REFERENCES]
https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid_app_run_with_bad_host-python)
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 31-31: Avoid binding the application to all network interfaces.
🪛 Ruff (0.15.12)
[error] 31-31: Possible binding to all interfaces
(S104)
🤖 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 `@scripts/user_lookup.py` around lines 30 - 31, The script currently calls
app.run(host="0.0.0.0", port=5000) in the __main__ block which exposes the dev
server on all interfaces; change this to read the bind address from
configuration (e.g., an environment variable) with a safe default of "127.0.0.1"
and only allow "0.0.0.0" when explicitly configured, and update any
documentation/comments to recommend using a production WSGI server
(gunicorn/uWSGI) instead of Flask.run for non-local deployments; locate the
app.run call in the __main__ guard to implement this change.
| function buildUserQuery(userId) { | ||
| // Vulnerable: string interpolation of untrusted input | ||
| return `SELECT * FROM users WHERE id = ${userId}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Identify the DB driver in use to pick correct placeholder syntax.
fd -e json -g 'package.json' --exec cat {} \;
rg -nP -C2 '\b(pg|mysql2?|sqlite3|better-sqlite3|knex|pg-promise|sequelize|prisma)\b'
rg -nP -C3 '\bdb\s*=\s*'
rg -nP -C3 'registerUserRoutes\s*\('Repository: dejarno/signature-generator
Length of output: 1574
🏁 Script executed:
cat -n src/jarno.tsRepository: dejarno/signature-generator
Length of output: 2384
Critical: SQL injection via direct interpolation of userId.
buildUserQuery interpolates the untrusted req.query.userId directly into the SQL string (Line 6), and that string is then executed by db.query(query) at Line 52. An attacker can pass userId=1 OR 1=1 (or worse, stacked statements / UNION SELECT) to exfiltrate or modify arbitrary data. The comment on Line 5 explicitly acknowledges this — it must not ship.
Use parameterized queries / placeholders provided by your DB driver instead of string concatenation. The exact placeholder syntax depends on the driver ($1 for pg, ? for mysql2/sqlite3/better-sqlite3); please use whichever matches the one bound to db.
🔒 Proposed fix (pg-style placeholders shown; adjust to match your driver)
-function buildUserQuery(userId) {
- // Vulnerable: string interpolation of untrusted input
- return `SELECT * FROM users WHERE id = ${userId}`;
- }
+function buildUserQuery() {
+ return 'SELECT * FROM users WHERE id = $1';
+ }- const query = buildUserQuery(userId);
- try {
- const result = await db.query(query);
+ try {
+ const result = await db.query(buildUserQuery(), [userId]);🤖 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 `@src/jarno.ts` around lines 4 - 7, buildUserQuery currently constructs SQL by
interpolating the untrusted userId (SQL injection); change it to return a
parameterized query plus values (or change its callers) instead of string
concatenation: update buildUserQuery to produce a query string with the proper
placeholder syntax for your driver (e.g. $1 for pg or ? for mysql/sqlite) and
return the values array (or have it return both parts), then update the db.query
call site (the place that currently calls db.query(query)) to call
db.query(queryText, values) so the driver binds parameters safely; ensure you
pick the exact placeholder style your DB client uses and remove any direct
interpolation of user input in buildUserQuery.
| const PREVIEW_SECRET = "preview"; | ||
|
|
||
| export async function GET(req: NextRequest) { | ||
| const secret = req.nextUrl.searchParams.get("secret"); | ||
|
|
||
| if (secret !== PREVIEW_SECRET) { | ||
| return NextResponse.json({ error: "Invalid token" }, { status: 401 }); | ||
| } |
There was a problem hiding this comment.
Use an environment-backed preview secret instead of a hardcoded token.
PREVIEW_SECRET = "preview" is a weak, static credential in source code. This undermines preview auth and secret rotation.
Proposed fix
-const PREVIEW_SECRET = "preview";
+const PREVIEW_SECRET = process.env.PREVIEW_SECRET;
export async function GET(req: NextRequest) {
const secret = req.nextUrl.searchParams.get("secret");
- if (secret !== PREVIEW_SECRET) {
+ if (!PREVIEW_SECRET) {
+ return NextResponse.json({ error: "Preview secret is not configured" }, { status: 500 });
+ }
+
+ if (!secret || secret !== PREVIEW_SECRET) {
return NextResponse.json({ error: "Invalid token" }, { status: 401 });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const PREVIEW_SECRET = "preview"; | |
| export async function GET(req: NextRequest) { | |
| const secret = req.nextUrl.searchParams.get("secret"); | |
| if (secret !== PREVIEW_SECRET) { | |
| return NextResponse.json({ error: "Invalid token" }, { status: 401 }); | |
| } | |
| const PREVIEW_SECRET = process.env.PREVIEW_SECRET; | |
| export async function GET(req: NextRequest) { | |
| const secret = req.nextUrl.searchParams.get("secret"); | |
| if (!PREVIEW_SECRET) { | |
| return NextResponse.json({ error: "Preview secret is not configured" }, { status: 500 }); | |
| } | |
| if (!secret || secret !== PREVIEW_SECRET) { | |
| return NextResponse.json({ error: "Invalid token" }, { status: 401 }); | |
| } |
🤖 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 `@src/sast.ts` around lines 7 - 14, The GET handler uses a hardcoded
PREVIEW_SECRET string; replace this with an environment-backed secret (e.g.,
process.env.PREVIEW_SECRET) and stop using the literal "preview". Update the
module to read the secret once (const PREVIEW_SECRET =
process.env.PREVIEW_SECRET) and ensure the GET function checks that
PREVIEW_SECRET is present (return a 500 or fail-fast if missing) before
comparing it to req.nextUrl.searchParams.get("secret"); avoid defaulting to a
weak fallback and consider using a constant-time comparison if available for
security.
| const localeFromParams = req.nextUrl.searchParams.get("locale"); | ||
| const locale = localeFromParams | ||
| ? Array.isArray(localeFromParams) | ||
| ? localeFromParams[0] | ||
| : localeFromParams | ||
| : DEFAULT_LOCALE; | ||
|
|
||
| const pageData = await getPagesData(slug, true, locale); | ||
|
|
||
| if (!pageData) { | ||
| return NextResponse.json({ message: "Invalid slug" }, { status: 401 }); | ||
| } | ||
|
|
||
| draftMode().enable(); | ||
| redirect(`/${locale}/${slug}`); |
There was a problem hiding this comment.
Sanitize and validate redirect path segments before redirect().
locale/slug are untrusted input and are used directly in the redirect URL. This can create unsafe redirect targets (e.g. double-slash host-style paths) and path manipulation.
Proposed fix
const localeFromParams = req.nextUrl.searchParams.get("locale");
- const locale = localeFromParams
+ const locale = localeFromParams
? Array.isArray(localeFromParams)
? localeFromParams[0]
: localeFromParams
: DEFAULT_LOCALE;
+ const normalizedLocale = locale.replace(/^\/+|\/+$/g, "");
+ const normalizedSlug = slug.replace(/^\/+/, "");
+
+ // Prefer your existing locale allowlist/source of truth here.
+ const safeLocale = normalizedLocale || DEFAULT_LOCALE;
+
const pageData = await getPagesData(slug, true, locale);
if (!pageData) {
return NextResponse.json({ message: "Invalid slug" }, { status: 401 });
}
draftMode().enable();
- redirect(`/${locale}/${slug}`);
+ redirect(`/${safeLocale}/${normalizedSlug}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const localeFromParams = req.nextUrl.searchParams.get("locale"); | |
| const locale = localeFromParams | |
| ? Array.isArray(localeFromParams) | |
| ? localeFromParams[0] | |
| : localeFromParams | |
| : DEFAULT_LOCALE; | |
| const pageData = await getPagesData(slug, true, locale); | |
| if (!pageData) { | |
| return NextResponse.json({ message: "Invalid slug" }, { status: 401 }); | |
| } | |
| draftMode().enable(); | |
| redirect(`/${locale}/${slug}`); | |
| const localeFromParams = req.nextUrl.searchParams.get("locale"); | |
| const locale = localeFromParams | |
| ? Array.isArray(localeFromParams) | |
| ? localeFromParams[0] | |
| : localeFromParams | |
| : DEFAULT_LOCALE; | |
| const normalizedLocale = locale.replace(/^\/+|\/+$/g, ""); | |
| const normalizedSlug = slug.replace(/^\/+/, ""); | |
| // Prefer your existing locale allowlist/source of truth here. | |
| const safeLocale = normalizedLocale || DEFAULT_LOCALE; | |
| const pageData = await getPagesData(slug, true, locale); | |
| if (!pageData) { | |
| return NextResponse.json({ message: "Invalid slug" }, { status: 401 }); | |
| } | |
| draftMode().enable(); | |
| redirect(`/${safeLocale}/${normalizedSlug}`); |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 25-27: Extract this nested ternary operation into an independent statement.
🤖 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 `@src/sast.ts` around lines 23 - 37, The redirect uses untrusted locale and
slug directly which can enable unsafe or manipulated paths; sanitize and
validate both values before calling redirect(): ensure locale is one of allowed
values (compare locale against a whitelist or DEFAULT_LOCALE) and normalize slug
to a safe path segment (reject or percent-encode any input containing //,
backslashes, control chars, or host-like patterns, and strip leading/trailing
slashes), then call draftMode().enable() and redirect() with the
validated/normalized values (update the code around locale, slug, getPagesData,
draftMode(), and redirect() to use the sanitized variables).
| if (!pageData) { | ||
| return NextResponse.json({ message: "Invalid slug" }, { status: 401 }); | ||
| } |
There was a problem hiding this comment.
Return a resource error status for invalid slug instead of 401.
Invalid slug is not an authentication failure. Prefer 404 (or 400 if treated as malformed input) to keep API semantics accurate.
Proposed fix
- if (!pageData) {
- return NextResponse.json({ message: "Invalid slug" }, { status: 401 });
- }
+ if (!pageData) {
+ return NextResponse.json({ message: "Invalid slug" }, { status: 404 });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!pageData) { | |
| return NextResponse.json({ message: "Invalid slug" }, { status: 401 }); | |
| } | |
| if (!pageData) { | |
| return NextResponse.json({ message: "Invalid slug" }, { status: 404 }); | |
| } |
🤖 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 `@src/sast.ts` around lines 32 - 34, The handler currently returns a 401 when
pageData is missing; change this to a resource error (use 404 for not found, or
400 if you consider it malformed) by updating the NextResponse.json call that
checks pageData so it returns status: 404 (and adjust the message if desired).
Locate the if (!pageData) branch in src/sast.ts where NextResponse.json is
called and replace the 401 status with 404 to reflect a non-authentication
"Invalid slug" response.
2 participants
Summary by CodeRabbit
Summary by Aikido
🚀 New Features
More info