Author page issue #103
Author page issue #103
Conversation
WalkthroughAdds conditional request/response logging to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant nativeRequest
participant HTTP
Client->>nativeRequest: call(params, { enableLog: true/false })
alt enableLog == true
nativeRequest->>HTTP: send request (logged)
HTTP-->>nativeRequest: full response
nativeRequest-->>nativeRequest: log response status/headers/body
nativeRequest-->>Client: return response.body
else enableLog == false
nativeRequest->>HTTP: send request
HTTP-->>nativeRequest: full response
nativeRequest-->>Client: return full response
end
alt HTTP returns error
nativeRequest-->>nativeRequest: log status (+ headers/content if present)
nativeRequest-->>Client: throw or propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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: 2
🧹 Nitpick comments (1)
package.json (1)
3-3: Reconsider the pre-release version format.The version
2.7.1-author-page-issue.0suggests this is a temporary diagnostic release. Pre-release versions with issue identifiers should not be published to production. Consider using a standard semantic version (e.g.,2.7.1) once the issue is resolved and the debugging code is removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
index.js(2 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
index.js (1)
collection-loader.js (1)
opts(33-33)
index.js
Outdated
| opts?.enableLog ? { resolveWithFullResponse: true } : {}, | ||
| opts | ||
| ); | ||
| return rp(params).catch(e => { | ||
| console.error(`Error in API ${uri}: Status ${e.statusCode}`); | ||
| return rp(params) | ||
| .then(response => { | ||
| if (opts?.enableLog) { | ||
| console.log("log--request params", params); | ||
| console.log(`log--response headers for ${uri}:`, response.headers); | ||
| console.log(`log--response content for ${uri}:`, response.body); | ||
| return response.body; | ||
| } | ||
| return response; | ||
| }) |
There was a problem hiding this comment.
Critical: Breaking change and security risks with conditional logging.
This implementation has several critical issues:
-
Breaking change: The function returns different types based on the
enableLogflag—response.bodywhen true vs. the fullresponseobject when false. This violates the principle of consistent function behavior and will break any code expecting the full response object. -
Security issue - sensitive data exposure: Logging
params,response.headers, andresponse.bodycan expose:- Authentication tokens (e.g.,
X-QT-AUTH,qt-internal-apps-keyin headers) - API keys and secrets
- PII data such as user emails, identifiers, and other personal information in the request/response body
This violates security best practices and compliance requirements (GDPR/CCPA).
- Authentication tokens (e.g.,
-
Formatting: Line 917 has inconsistent indentation with extra spaces.
Consider these alternatives:
Option 1 (Recommended): Remove the logging entirely and use proper observability tools (APM, structured logging with PII redaction) for debugging production issues.
Option 2: If temporary debugging is needed, ensure:
- Return value remains consistent (always return the same type)
- Sensitive fields are redacted before logging
- The feature is controlled by an environment variable, not hardcoded
- Add clear documentation that this is for debugging only
Apply this diff to maintain consistent return behavior:
const params = Object.assign(
{
method: "GET",
uri: uri,
json: true,
gzip: true
},
- opts?.enableLog ? { resolveWithFullResponse: true } : {},
opts
);
return rp(params)
- .then(response => {
- if (opts?.enableLog) {
- console.log("log--request params", params);
- console.log(`log--response headers for ${uri}:`, response.headers);
- console.log(`log--response content for ${uri}:`, response.body);
- return response.body;
- }
- return response;
- })
+ .then(response => response)
.catch(e => {
console.error(`Error in API ${uri}: Status ${e.statusCode}`);
- if (e.response) {
- console.error(`log--error headers for ${uri}:`, e.response.headers);
- console.error(`log--error content for ${uri}:`, e.response.body);
- }
throw e;
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In index.js around lines 907 to 919, the conditional logging block both changes
the function's return type (returning response.body when opts.enableLog is true
vs the full response when false) and logs sensitive request/response data; to
fix, make the function always return the same value (the full response)
regardless of logging, remove or replace the raw console.logs with a controlled
debug path behind an environment flag, redact sensitive headers/body fields
before any logging (eg. mask tokens, API keys, emails, PII), and correct the
inconsistent indentation on line 917; prefer removing logs entirely or routing
through a structured logger that supports PII redaction and is disabled in
production.
| getAuthorCollection(authorId, params) { | ||
| return this.request(`/api/v1/authors/${authorId}/collection`, { | ||
| qs: params | ||
| qs: params, | ||
| enableLog: true | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: Hardcoded debug flag enables unsafe logging in production.
The enableLog: true flag is hardcoded, which means:
- All author collection requests will trigger verbose logging (including sensitive headers and request/response bodies)
- This cannot be disabled in production without a code change
- Combined with the logging implementation in
nativeRequest, this will expose sensitive data for every author collection API call
Debug flags should never be hardcoded to true in production code.
Recommended approach: Remove this temporary debugging code entirely:
getAuthorCollection(authorId, params) {
return this.request(`/api/v1/authors/${authorId}/collection`, {
- qs: params,
- enableLog: true
+ qs: params
});
}If debugging is genuinely needed, control it via an environment variable:
getAuthorCollection(authorId, params) {
return this.request(`/api/v1/authors/${authorId}/collection`, {
qs: params,
- enableLog: true
+ ...(process.env.DEBUG_AUTHOR_COLLECTION === 'true' && { enableLog: true })
});
}However, given the security issues with the logging implementation, Option 1 (complete removal) is strongly recommended.
📝 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.
| getAuthorCollection(authorId, params) { | |
| return this.request(`/api/v1/authors/${authorId}/collection`, { | |
| qs: params | |
| qs: params, | |
| enableLog: true | |
| }); | |
| } | |
| getAuthorCollection(authorId, params) { | |
| return this.request(`/api/v1/authors/${authorId}/collection`, { | |
| qs: params | |
| }); | |
| } |
| getAuthorCollection(authorId, params) { | |
| return this.request(`/api/v1/authors/${authorId}/collection`, { | |
| qs: params | |
| qs: params, | |
| enableLog: true | |
| }); | |
| } | |
| getAuthorCollection(authorId, params) { | |
| return this.request(`/api/v1/authors/${authorId}/collection`, { | |
| qs: params, | |
| ...(process.env.DEBUG_AUTHOR_COLLECTION === 'true' && { enableLog: true }) | |
| }); | |
| } |
🤖 Prompt for AI Agents
In index.js around lines 1185 to 1190, the request call to
/api/v1/authors/${authorId}/collection hardcodes enableLog: true which forces
verbose/sensitive logging in production; remove the enableLog property entirely
from this call (Option 1) so the request uses the default logging behavior,
ensure no other code paths reintroduce a hardcoded true, and if debugging is
required implement a controlled switch that reads an environment variable (e.g.,
process.env.ENABLE_REQUEST_LOGGING === 'true') before setting enableLog so
logging can be toggled without code changes; update any tests or documentation
referencing this flag.
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
Chores