Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

just logging the entire req from Nginx #3542

Merged
merged 1 commit into from
Sep 29, 2024
Merged

just logging the entire req from Nginx #3542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/auth-service/bin/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ require("module-alias/register");
const dotenv = require("dotenv");
dotenv.config();
require("app-module-path").addPath(__dirname);
const kafkaConsumer = require("./kafka-consumer");
const kafkaConsumer = require("@bin/jobs/kafka-consumer");
const createServer = require("./server");
const log4js = require("log4js");
const constants = require("@config/constants");
Expand Down
2 changes: 1 addition & 1 deletion src/auth-service/bin/active-status-job.js → ...uth-service/bin/jobs/active-status-job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const constants = require("@config/constants");
const inactiveThreshold = constants.INACTIVE_THRESHOLD || 2592000000; // 30 days
const log4js = require("log4js");
const logger = log4js.getLogger(
`${constants.ENVIRONMENT} -- bin/active-status-job script`
`${constants.ENVIRONMENT} -- bin/jobs/active-status-job script`
);
const stringify = require("@utils/stringify");

Expand Down
2 changes: 1 addition & 1 deletion ...uth-service/bin/incomplete-profile-job.js → ...ervice/bin/jobs/incomplete-profile-job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const UserModel = require("@models/User");
const constants = require("@config/constants");
const log4js = require("log4js");
const logger = log4js.getLogger(
`${constants.ENVIRONMENT} -- bin/incomplete-profile-job`
`${constants.ENVIRONMENT} -- bin/jobs/incomplete-profile-job`
);
const stringify = require("@utils/stringify");
const mailer = require("@utils/mailer");
Expand Down
2 changes: 1 addition & 1 deletion src/auth-service/bin/kafka-consumer.js → src/auth-service/bin/jobs/kafka-consumer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const { Kafka } = require("kafkajs");
const constants = require("@config/constants");
const log4js = require("log4js");
const logger = log4js.getLogger(
`${constants.ENVIRONMENT} -- bin/kafka-consumer script`
`${constants.ENVIRONMENT} -- bin/jobs/kafka-consumer script`
);
const { logObject } = require("@utils/log");
const mailer = require("@utils/mailer");
Expand Down
2 changes: 1 addition & 1 deletion ...uth-service/bin/test/ut_kafka-consumer.js → ...ervice/bin/jobs/test/ut_kafka-consumer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require("module-alias/register");
const { expect } = require("chai");
const sinon = require("sinon");
const kafkaConsumer = require("@bin/kafka-consumer");
const kafkaConsumer = require("@bin/jobs/kafka-consumer");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Remaining Instances of the Old Import Path Detected

The following files still use the old import path "@bin/kafka-consumer":

  • src/incentives/bin/test/ut_kafka-consumer.js
  • src/incentives/bin/test/ut_index.js
  • src/blog-content-manager/bin/test/ut_kafka-consumer.js
  • src/blog-content-manager/bin/test/ut_index.js
  • src/auth-service/bin/test/ut_index.js

Please update these import statements to "@bin/jobs/kafka-consumer" to maintain consistency across the codebase.

🔗 Analysis chain

Excellent restructuring of the kafka-consumer module!

The updated import path reflects a more organized project structure, which is a positive change. The kafka-consumer module is now appropriately placed within the 'jobs' subdirectory.

To ensure this change doesn't introduce any issues elsewhere in the codebase, let's verify other occurrences of the old import path:

If the first command returns any results, those files may need to be updated. The second command should confirm the existence of the kafka-consumer.js file in the new location.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the old import path

# Test: Search for the old import path. Expect: No results.
rg --type js '"@bin/kafka-consumer"'

# Test: Verify the new module location exists
fd -t f kafka-consumer.js src/auth-service/bin/jobs

Length of output: 1016


// Mock the required modules/functions
const constants = require("@config/constants");
Expand Down
2 changes: 1 addition & 1 deletion src/auth-service/bin/token-expiration-job.js → ...-service/bin/jobs/token-expiration-job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const mailer = require("@utils/mailer");
const stringify = require("@utils/stringify");
const log4js = require("log4js");
const logger = log4js.getLogger(
`${constants.ENVIRONMENT} -- bin/token-expiration-job`
`${constants.ENVIRONMENT} -- bin/jobs/token-expiration-job`
);
const moment = require("moment-timezone");

Expand Down
6 changes: 3 additions & 3 deletions src/auth-service/bin/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
const isDev = process.env.NODE_ENV === "development";
const isProd = process.env.NODE_ENV === "production";
const options = { mongooseConnection: mongoose.connection };
require("@bin/active-status-job");
require("@bin/token-expiration-job");
require("@bin/incomplete-profile-job");
require("@bin/jobs/active-status-job");
require("@bin/jobs/token-expiration-job");
require("@bin/jobs/incomplete-profile-job");

Check warning on line 24 in src/auth-service/bin/server.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/bin/server.js#L22-L24 

Added lines #L22 - L24 were not covered by tests
const log4js = require("log4js");
const debug = require("debug")("auth-service:server");
const isEmpty = require("is-empty");
Expand Down
3 changes: 2 additions & 1 deletion src/auth-service/middleware/passport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,8 @@ const useJWTStrategy = (tenant, req, res, next) =>
const endpoint = req.headers["x-original-uri"];
const clientOriginalIp = req.headers["x-client-original-ip"];
const requestBody = req.body;
logObject("Request Body:", requestBody);
logObject("Request Body", requestBody);
logObject("logging the entire req", req);
Comment on lines +377 to +378
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider security and performance implications of extensive logging.

The addition of these logging statements aligns well with the PR objective of implementing logging for the entire request received from Nginx. This enhancement will undoubtedly improve our ability to debug and monitor incoming requests. However, I have a few friendly suggestions to ensure we're implementing this feature in the most effective and secure manner:

  1. Security: Logging the entire request object might inadvertently expose sensitive information. Consider implementing a sanitization step to remove or mask sensitive data before logging.

  2. Timing: These logging statements are placed at the beginning of the function, before any authentication checks. You might want to consider moving them after initial validations to avoid unnecessary logging of unauthorized requests.

  3. Performance: The logObject function might not be optimized for large objects like the entire request. Consider using a streaming or chunked logging approach for large objects to minimize memory usage.

Here's a potential refactoring to address these points:

- logObject("Request Body", requestBody);
- logObject("logging the entire req", req);
+ const sanitizedReq = sanitizeRequest(req);
+ logObject("Sanitized Request Body", sanitizedReq.body);
+ if (process.env.NODE_ENV === 'development') {
+   logObject("Full sanitized request (dev only)", sanitizedReq);
+ }

function sanitizeRequest(req) {
  // Implement sanitization logic here
  // e.g., remove sensitive headers, mask PII in body, etc.
}

This approach sanitizes the request, always logs the body (which is likely smaller), and only logs the full request in development environments. Remember to implement the sanitizeRequest function to remove or mask sensitive data!

Committable suggestion was skipped due to low confidence.


let service = req.headers["service"] || "unknown";
let userAction = "unknown";
Expand Down
Loading